Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Nov 2, 2020

Conversation

cjcenizal
Copy link
Contributor

@cjcenizal cjcenizal commented Oct 29, 2020

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 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
Contributor

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

Copy link
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
Contributor Author

@elasticmachine merge upstream

@cjcenizal
Copy link
Contributor Author

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?

Copy link
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
Contributor Author

Thanks @ryankeairns! Good to know. 👍

@yuliacech
Copy link
Contributor

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

@cjcenizal
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 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
cjcenizal added a commit that referenced this pull request Nov 2, 2020
…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
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 v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants