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

[SearchProfiler] SearchProfiler to NP #48795

Merged
merged 42 commits into from
Oct 31, 2019

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Oct 21, 2019

Summary

Search profiler new platform migration.

General

  • NP Migration review (shims)
  • Copy Review
  • a11y
  • IE & FF & Chrome & Safari

Public

  • Finish public migration (done)
  • Fix vertical scrolling in the profile viz pane
  • Fix the Ace integration (showing error lines in output)
  • Delete old code!
Screenshots old Screenshot 2019-10-24 at 12 35 33 Screenshot 2019-10-24 at 12 35 47 Screenshot 2019-10-24 at 12 35 50
Screenshots new Screenshot 2019-10-24 at 17 48 20 Screenshot 2019-10-24 at 17 48 27 Screenshot 2019-10-24 at 17 48 33 Screenshot 2019-10-24 at 17 48 41
New states (w/ new copy) Screenshot 2019-10-28 at 14 16 18 Screenshot 2019-10-28 at 14 16 28

Server

  • Finish server migration

How to review

The focus here has been on re-architecting the existing NG app into React+TS+EUI.

State Management
Because the app doesn't have much app state we are not introducing any more heavy state management tools (just React Context for sharing more general dependencies).

UI
There are some custom SCSS shenanigans going on here, primarily the profile tree visualization. I've also organised the SCSS files by component and containers. So the styles directory more or less mirrors the TS files.

UX
It's important that we, for the most part preserve pre-existing behaviour:
Check that the UI updates as expected after:
1. A request is made
2. An index (or sub component) is expanded or collapsed. It would be really good to test with deep nesting here (what are good queries we can test with here?)
3. Test weird edge case combinations. E.g., What if we send a request with the flyout open?

Where possible we want to improve UX and UI. Suggestions from design or ES UI are welcome!

UI/UX improvements will probably be done on a separate PR

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@jloleysens jloleysens marked this pull request as ready for review October 24, 2019 15:51
@jloleysens jloleysens changed the title [WiP][SearchProfiler] SearchProfiler to NP [SearchProfiler] SearchProfiler to NP Oct 24, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💔 Build Failed

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💔 Build Failed

@jloleysens
Copy link
Contributor Author

Retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Contributor

@miukimiu miukimiu left a comment

Choose a reason for hiding this comment

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

We agreed to merge this PR and create a new PR with UX/UI improvements from the issue #49772

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@jloleysens jloleysens merged commit e385015 into elastic:master Oct 31, 2019
@jloleysens jloleysens deleted the np-migration/searchprofiler branch October 31, 2019 21:23
jloleysens added a commit to jloleysens/kibana that referenced this pull request Oct 31, 2019
* Added the following components:

- highlight details (with render test)
- searchprofiler tabs
- (wip) profile tree
- (wip) shard details

* First iteration of ProfileTree component (needs render test)

* Remove space

* ProfileTree render test

* Add profile tree test to git index

* First iteration of editor component with render test

* First iteration of nearly functional public

* Fix highlight_details_flyout render test

* Move NP directory to public and fix import issue created by directly importing FormattedMessage

* Rendering and looking more normal

* Fix type issues and fix a11y for ace editor

* Added ability to do profile requests again and render into UI (styling WiP)

* Fix props in editor test

* Added empty tree placeholder component (with test), moved styling around into individual files (wip)

* Fix path

* Lots of style updates and added util for determining visible children (+test)

* Re-add missing badge and make it slightly wider (otherwise 100.00% cuts off to 100.0...)

* Delete legacy public!

* SCSS refactor + fix for re-rendering editor

* UI and server updates after license checks

* [skip ci] Add server np_ready code

* fix i18n

* Re-enable error annotations

* Minor UX improvements (focus editor after failed request and no tabindex for textarea without active license)
Added some spaces to make code more readable

* Removed xpackMain from ServerShim
Updated use of notifications -> notifications.toasts from np core setup
Removed TODO for using core.application.register (not available for legacy apps)

* Added placeholder component for loading state and implemented useReducer

* Refactor actions

* Changes after PR feedback:
- TS for unsafe utils test fixtures
- Safer use of .selfTime (no more NaN)
- Sentence case where applicable
- Cleaned up TODOs
- Fix styling issue with percentage on badges of profile tree
- Refactor name of profile hook (now useRequestProfile)
- Fixed copy paste issue in highlight flyout `Total time` -> `Self time`
- Restyled the profile button to be fill and not take up the full horizontal space
- Removed the `Type` input from the profiler query section

* Removed .type from backend and cleanup translations

* Disable responsive UI layout for now

* Remove buggy error annotation code

* - Refactored percentage badge to own component
- Updated styles after testing on IE11
- Updated styles after testing on Safari
- Chrome and FF worked on this commit

* Update missing i18n and fix use ace ui keyboard hook

* Update useEffect dependencies array for editor component

* Use absolute path to dev tools app (to fix CI)

* Remove file extensions

* Re-add missing data-test-subj
jloleysens added a commit that referenced this pull request Nov 1, 2019
* [SearchProfiler] SearchProfiler to NP (#48795)

* Added the following components:

- highlight details (with render test)
- searchprofiler tabs
- (wip) profile tree
- (wip) shard details

* First iteration of ProfileTree component (needs render test)

* Remove space

* ProfileTree render test

* Add profile tree test to git index

* First iteration of editor component with render test

* First iteration of nearly functional public

* Fix highlight_details_flyout render test

* Move NP directory to public and fix import issue created by directly importing FormattedMessage

* Rendering and looking more normal

* Fix type issues and fix a11y for ace editor

* Added ability to do profile requests again and render into UI (styling WiP)

* Fix props in editor test

* Added empty tree placeholder component (with test), moved styling around into individual files (wip)

* Fix path

* Lots of style updates and added util for determining visible children (+test)

* Re-add missing badge and make it slightly wider (otherwise 100.00% cuts off to 100.0...)

* Delete legacy public!

* SCSS refactor + fix for re-rendering editor

* UI and server updates after license checks

* [skip ci] Add server np_ready code

* fix i18n

* Re-enable error annotations

* Minor UX improvements (focus editor after failed request and no tabindex for textarea without active license)
Added some spaces to make code more readable

* Removed xpackMain from ServerShim
Updated use of notifications -> notifications.toasts from np core setup
Removed TODO for using core.application.register (not available for legacy apps)

* Added placeholder component for loading state and implemented useReducer

* Refactor actions

* Changes after PR feedback:
- TS for unsafe utils test fixtures
- Safer use of .selfTime (no more NaN)
- Sentence case where applicable
- Cleaned up TODOs
- Fix styling issue with percentage on badges of profile tree
- Refactor name of profile hook (now useRequestProfile)
- Fixed copy paste issue in highlight flyout `Total time` -> `Self time`
- Restyled the profile button to be fill and not take up the full horizontal space
- Removed the `Type` input from the profiler query section

* Removed .type from backend and cleanup translations

* Disable responsive UI layout for now

* Remove buggy error annotation code

* - Refactored percentage badge to own component
- Updated styles after testing on IE11
- Updated styles after testing on Safari
- Chrome and FF worked on this commit

* Update missing i18n and fix use ace ui keyboard hook

* Update useEffect dependencies array for editor component

* Use absolute path to dev tools app (to fix CI)

* Remove file extensions

* Re-add missing data-test-subj

* Remove template/index.html
@cjcenizal cjcenizal added Feature:NP Migration Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more labels Apr 22, 2020
@elasticmachine
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Dev Tools Feature:NP Migration Feature:Search Profiler Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants