Skip to content

Conform Search Profiler application organization to other ES UI plugins#82085

Merged
cjcenizal merged 7 commits intoelastic:masterfrom
cjcenizal:chore/search-profiler-org
Nov 2, 2020
Merged

Conform Search Profiler application organization to other ES UI plugins#82085
cjcenizal merged 7 commits intoelastic:masterfrom
cjcenizal:chore/search-profiler-org

Conversation

@cjcenizal
Copy link
Copy Markdown
Contributor

@cjcenizal cjcenizal commented Oct 29, 2020 •
edited
Loading

This PR just moves files around, so all that's needed is verification that the folder structure looks good, and that the app works in the browser. This PR also updates the README with steps for testing.

@cjcenizal cjcenizal added chore v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// release_note:skip Skip the PR/issue when compiling release notes Feature:Search Profiler v7.11.0 labels Oct 29, 2020
@cjcenizal cjcenizal requested a review from yuliacech October 29, 2020 21:42
@cjcenizal cjcenizal requested a review from a team as a code owner October 29, 2020 21:42
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Oct 29, 2020

Pinging @elastic/es-ui (Team:Elasticsearch UI)

yuliacech
yuliacech approved these changes Oct 30, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@yuliacech yuliacech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @cjcenizal , thanks a lot for updating this plugin! I tested locally and all worked for me.
The new folder structure is definitely more similar to other plugins. A couple of non-blocking suggestions:

  • renaming utils to lib as this seems to be used more often in our plugins
  • moving files out of contexts folder into the root of application so that both contexts are close to index and app where they are used

@cjcenizal cjcenizal requested a review from a team as a code owner October 30, 2020 18:58
@cjcenizal
Copy link
Copy Markdown
Contributor Author

cjcenizal commented Oct 30, 2020

@elasticmachine merge upstream

@cjcenizal
Copy link
Copy Markdown
Contributor Author

cjcenizal commented Oct 30, 2020

Thanks for the review @yuliacech! I implemented your suggestion of renaming utils to lib, but I left the contexts directory alone because I find it easier to identify module entry points and relationships if there are a minimal number of files in the root. However, I did realize that the styles directory is unorthodox, so I reorganized the styles to live alongside the components they decorate. Could you please take another look?

ryankeairns
ryankeairns approved these changes Oct 30, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this plugin has gone the extra mile with the use of _index.scss files :)

In most places, we see one SCSS file per .../component/xxxx/ directory and it matches the component name. Regardless, not an issue, just an observation for which I might be missing additional reasoning.

@cjcenizal
Copy link
Copy Markdown
Contributor Author

cjcenizal commented Oct 30, 2020

Thanks @ryankeairns! Good to know. 👍

@yuliacech
Copy link
Copy Markdown
Contributor

yuliacech commented Nov 2, 2020

Hi @cjcenizal , tested locally with the new changes and everything worked for me. I haven't noticed any visual regressions either!

@cjcenizal
Copy link
Copy Markdown
Contributor Author

cjcenizal commented Nov 2, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Nov 2, 2020

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id before after diff
searchprofiler 85 89 +4

async chunks size

id before after diff
searchprofiler 691.5KB 690.7KB -871.0B

distributable file count

id before after diff
default 48218 48221 +3

page load bundle size

id before after diff
searchprofiler 49.8KB 48.3KB -1.6KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@cjcenizal cjcenizal merged commit a83b8ff into elastic:master Nov 2, 2020
@cjcenizal cjcenizal deleted the chore/search-profiler-org branch November 2, 2020 20:01
This was referenced Nov 2, 2020
Merged
Merged
cjcenizal added a commit that referenced this pull request Nov 2, 2020
@cjcenizal
Conform Search Profiler application organization to other ES UI plugi…
ebbd2db 
…ns (#82085) (#82365)

* Add testing steps to README.
* Update Search Profiler application organization to conform to organization of other ES UI plugins.
* Organize styles to live alongside the components they decorate.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ryankeairns ryankeairns ryankeairns approved these changes

+1 more reviewer

@yuliacech yuliacech yuliacech approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

chore Feature:Search Profiler release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// v7.11.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@cjcenizal @elasticmachine @yuliacech @kibanamachine @ryankeairns