ui/render_complete 👉 src/plugins/kibana_utils #44743

sainthkh · 2019-09-04T04:01:36Z

Summary

Moves ui/render_complete -> src/plugins/kibana_utils.

Notes & Questions

ui/render_complete 👉 src/plugins/kibana_utils #44367 said to move index.ts to kibana_utils. But it's impossible because it should import './directive. Should I inline directive.js to index.ts?
kibana_utils doesn't have kibana.json. Is it intended?

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~- [ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~- [ ] Documentation was added for features that require explanation or tutorials~~

Unit or functional tests were updated or added to match the most common scenarios
~~- [ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

elasticmachine · 2019-09-04T04:01:38Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

ppisljar · 2019-09-04T10:19:21Z

jenkins, test this

elasticmachine · 2019-09-04T10:27:20Z

Pinging @elastic/kibana-app-arch

ppisljar · 2019-09-04T10:30:47Z

the directive doesn't need to be imported in the index.ts
in this PR i would move to utils and leave directive.js behind, and create new index.ts which just imports the directive.js
also it would be nice if we can list all the places where we use the renderComplete directive here so we can see when can it be removed or if there is just a single place we could move it there.

elasticmachine · 2019-09-04T11:43:46Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

sainthkh · 2019-09-04T23:29:15Z

@ppisljar

You're right. index.ts should be removed and directive should be imported directly from new index.ts. This change made functional test pass.

When we search the term render-complete, there are 3 places where this directive is used.

src/legacy/core_plugins/kibana/public/discover/doc_table/doc_table.js

src/plugins/kibana_utils/public/render_complete/index.ts

ppisljar

mostly LGTM, left two minor cmments

src/legacy/core_plugins/kibana/public/discover/directives/index.js

src/legacy/ui/public/vis/vis.js

ppisljar · 2019-09-05T10:12:31Z

jenkins, test this

elasticmachine · 2019-09-05T11:29:37Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

mattkime · 2019-09-05T13:50:43Z

kibana_utils doesn't have kibana.json. Is it intended?

Yes, because its only providing static code.

lizozom · 2019-09-05T16:27:17Z

Looking at this PR, will we need this in our React code?

If not, it should be left in ui/public. If so, it shouldn't be in kibana_react plugin, but rather in visualize or embeddables.

What do you think @gammon ?

… index.ts.

sainthkh · 2019-09-06T01:56:03Z

src/legacy/core_plugins/kibana/public/discover/doc_table/doc_table.js

@@ -29,8 +29,7 @@ import './lib/pager';

 import { getLimitedSearchResultsMessage } from './doc_table_strings';

-uiModules
-  .get('app/discover')
+uiModules.get('app/discover')


@mattkime I reverted to old code. But according to the JavaScript Styleguide, it is recommended to use prettier. I guess that the our goal is to change the entire code base with prettier like the previous version of this commit.

Later, I should break the commit when there are changes by prettier to help others distinguish what's done by me and what's done by prettier.

If you're new to prettier, you can learn how to set up in this article. You'll soon wonder how you wrote code without this.

(Edit: I removed note because it's somewhat fixed after reinstall.)

sainthkh · 2019-09-10T23:28:34Z

Looking at this PR, will we need this in our React code?

If not, it should be left in ui/public. If so, it shouldn't be in kibana_react plugin, but rather in visualize or embeddables.

What do you think gammon ?

@lizozom asked a question but ended with a wrong name tag and this callout didn't reach you @stacey-gammon. So, I copied it here.

stacey-gammon · 2019-09-11T14:40:19Z

thanks @sainthkh. I think @mattkime is looking into how to replicate this via embeddables. So maybe this is on hold till that is figured out?

ppisljar · 2019-09-12T05:52:30Z

i think this can be moved without affecting @mattkime work. if in the end he finishes up with solution that would still use this helpers he would just import them from new place. If he ends up with something that doesn't need this anymore, then we can remove it at that point.

ppisljar · 2019-09-23T09:41:09Z

retest

ppisljar · 2019-09-23T09:41:44Z

@elasticmachine merge upstream

elasticmachine · 2019-09-23T11:13:09Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

sainthkh · 2019-09-23T22:51:12Z

@ppisljar This failure is a bit weird. When I ran it on my local Ubuntu, it failed at the first try. But after that, it passed 10 times without fail.

ppisljar · 2019-09-24T08:08:27Z

retest

elasticmachine · 2019-09-24T09:49:32Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

sainthkh · 2019-09-24T10:08:39Z

@ppisljar Another flaky test. I've run the test 5 times on local, it all passed.

ppisljar · 2019-09-24T12:12:06Z

retest

elasticmachine · 2019-09-24T13:58:25Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

ppisljar · 2019-09-25T12:22:47Z

retest

elasticmachine · 2019-09-25T14:11:54Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* Moved ui/render_complete to kibana_utils.

sainthkh requested a review from a team as a code owner September 4, 2019 04:01

ppisljar added release_note:skip Skip the PR/issue when compiling release notes review Team:AppArch v7.5.0 v8.0.0 labels Sep 4, 2019

sainthkh force-pushed the move-render-complete branch from c4884f2 to 1d579a5 Compare September 4, 2019 23:09

mattkime requested changes Sep 5, 2019

View reviewed changes

src/legacy/core_plugins/kibana/public/discover/doc_table/doc_table.js Outdated Show resolved Hide resolved

src/plugins/kibana_utils/public/render_complete/index.ts Outdated Show resolved Hide resolved

ppisljar approved these changes Sep 5, 2019

View reviewed changes

src/legacy/core_plugins/kibana/public/discover/directives/index.js Outdated Show resolved Hide resolved

src/legacy/ui/public/vis/vis.js Outdated Show resolved Hide resolved

sainthkh force-pushed the move-render-complete branch from 58237af to 5b575b9 Compare September 5, 2019 10:53

streamich mentioned this pull request Sep 5, 2019

Visualizations 👉 New Platform #44121

Closed

13 tasks

sainthkh added 5 commits September 6, 2019 07:55

Moved ui/render_complete to kibana_utils.

f8d7304

Removed ui/render_complete/index.ts and imported directive inside new…

9d87f3d

… index.ts.

Moved imported directive from render_complete in kibana_utils.

eb8c69a

Change to relative paths.

7e5ba77

Reverted prettier changes.

90e72de

sainthkh force-pushed the move-render-complete branch from 5b575b9 to 90e72de Compare September 5, 2019 23:01

sainthkh commented Sep 6, 2019

View reviewed changes

Merge branch 'master' into move-render-complete

82e7c25

Merge branch 'master' into move-render-complete

326fbb1

Merge branch 'master' into move-render-complete

6162a16

Merge remote-tracking branch 'upstream/master' into move-render-complete

ccdc126

Merge remote-tracking branch 'upstream/master' into move-render-complete

ba9a186

mattkime approved these changes Sep 25, 2019

View reviewed changes

mattkime merged commit f260466 into elastic:master Sep 25, 2019

mattkime pushed a commit to mattkime/kibana that referenced this pull request Sep 25, 2019

ui/render_complete 👉 src/plugins/kibana_utils (elastic#44743)

3d1fd2d

* Moved ui/render_complete to kibana_utils.

mattkime mentioned this pull request Sep 25, 2019

[7.x] ui/render_complete 👉 src/plugins/kibana_utils (#44743) #46597

Merged

mattkime added a commit that referenced this pull request Sep 26, 2019

ui/render_complete 👉 src/plugins/kibana_utils (#44743) (#46597)

8a777fd

* Moved ui/render_complete to kibana_utils.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ui/render_complete 👉 src/plugins/kibana_utils #44743

ui/render_complete 👉 src/plugins/kibana_utils #44743

sainthkh commented Sep 4, 2019 •

edited by ppisljar

Loading

elasticmachine commented Sep 4, 2019

ppisljar commented Sep 4, 2019

elasticmachine commented Sep 4, 2019

ppisljar commented Sep 4, 2019

elasticmachine commented Sep 4, 2019

sainthkh commented Sep 4, 2019

ppisljar left a comment

ppisljar commented Sep 5, 2019

elasticmachine commented Sep 5, 2019

mattkime commented Sep 5, 2019

lizozom commented Sep 5, 2019

sainthkh Sep 6, 2019 •

edited

Loading

sainthkh commented Sep 10, 2019

stacey-gammon commented Sep 11, 2019

ppisljar commented Sep 12, 2019

ppisljar commented Sep 23, 2019

ppisljar commented Sep 23, 2019

elasticmachine commented Sep 23, 2019

sainthkh commented Sep 23, 2019

ppisljar commented Sep 24, 2019

elasticmachine commented Sep 24, 2019

sainthkh commented Sep 24, 2019

ppisljar commented Sep 24, 2019

elasticmachine commented Sep 24, 2019

ppisljar commented Sep 25, 2019

elasticmachine commented Sep 25, 2019

ui/render_complete 👉 src/plugins/kibana_utils #44743

ui/render_complete 👉 src/plugins/kibana_utils #44743

Conversation

sainthkh commented Sep 4, 2019 • edited by ppisljar Loading

Summary

Notes & Questions

Checklist

For maintainers

elasticmachine commented Sep 4, 2019

ppisljar commented Sep 4, 2019

elasticmachine commented Sep 4, 2019

ppisljar commented Sep 4, 2019

elasticmachine commented Sep 4, 2019

💔 Build Failed

sainthkh commented Sep 4, 2019

ppisljar left a comment

Choose a reason for hiding this comment

ppisljar commented Sep 5, 2019

elasticmachine commented Sep 5, 2019

💚 Build Succeeded

mattkime commented Sep 5, 2019

lizozom commented Sep 5, 2019

sainthkh Sep 6, 2019 • edited Loading

Choose a reason for hiding this comment

sainthkh commented Sep 10, 2019

stacey-gammon commented Sep 11, 2019

ppisljar commented Sep 12, 2019

ppisljar commented Sep 23, 2019

ppisljar commented Sep 23, 2019

elasticmachine commented Sep 23, 2019

💔 Build Failed

sainthkh commented Sep 23, 2019

ppisljar commented Sep 24, 2019

elasticmachine commented Sep 24, 2019

💔 Build Failed

sainthkh commented Sep 24, 2019

ppisljar commented Sep 24, 2019

elasticmachine commented Sep 24, 2019

💚 Build Succeeded

ppisljar commented Sep 25, 2019

elasticmachine commented Sep 25, 2019

💚 Build Succeeded

sainthkh commented Sep 4, 2019 •

edited by ppisljar

Loading

sainthkh Sep 6, 2019 •

edited

Loading