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.

Sign up for GitHub

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

Jump to bottom

[Discover] Create field_button and add popovers to sidebar #73226

Merged
merged 53 commits into from
Aug 18, 2020
Merged

[Discover] Create field_button and add popovers to sidebar #73226

merged 53 commits into from
Aug 18, 2020

Conversation

andreadelrio
Copy link
Contributor

@andreadelrio andreadelrio commented Jul 25, 2020 •
edited
Loading

Summary

  • Created new shared component based on Lens' field_item called field_button. Includes improvements to a11y navigation
  • [Discover] Replaced accordions on sidebar with popovers. Implemented field_button.
  • [Lens] Implemented field_button.

aGroup4

update_pops

Related to #41108

Checklist

Delete any items that are not applicable to this PR.

- [ ] 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

For maintainers

@andreadelrio andreadelrio changed the title [Discover] Add popovers to sidebar [Discover] Create field_button and add popovers to sidebar Jul 28, 2020
@andreadelrio andreadelrio marked this pull request as ready for review July 28, 2020 15:53
@andreadelrio andreadelrio requested a review from a team July 28, 2020 15:53
@andreadelrio andreadelrio requested review from a team as code owners July 28, 2020 15:53
@andreadelrio andreadelrio added release_note:skip Skip the PR/issue when compiling release notes v8.0.0 v7.10.0 Feature:Discover Discover Application Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Jul 28, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@kertal kertal self-requested a review July 29, 2020 06:28
@kertal
Copy link
Member

kertal commented Jul 30, 2020

@elasticmachine merge upstream

@andreadelrio
Copy link
Contributor Author

@cchaos This currently breaks when the index pattern has fields of type unknown and I'll likely need Matthias help to fix it properly but this is how the design looks with icon buttons. You can use kibana_sample_data_logs to see it locally.

aGroup4

@cchaos
Copy link
Contributor

cchaos commented Aug 13, 2020

@elasticmachine merge upstream

cchaos
cchaos approved these changes Aug 14, 2020
View reviewed changes
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Looks great to me! 🎉

I only had one comment on the state of showing the add/remove button when the button is active. Other than that and then getting CI to pass 👍

src/plugins/discover/public/application/components/sidebar/discover_sidebar.scss Outdated Show resolved Hide resolved
@andreadelrio
Copy link
Contributor Author

@elasticmachine merge upstream

@andreadelrio
Copy link
Contributor Author

jenkins test this

@kertal kertal mentioned this pull request Aug 18, 2020
Merged
3 tasks
kertal
kertal approved these changes Aug 18, 2020
View reviewed changes
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Code LGTM, great work 🥳 , added some minor comments. Tested in Chrome 83, Firefox 79, Safari 13.1.2, no regression detected. there's a minor change in Lens that I've noticed, navigating the field list with tabs doesn't show the info icon:
Bildschirmfoto 2020-08-18 um 16 12 49

@cchaos
Copy link
Contributor

cchaos commented Aug 18, 2020

in Lens that I've noticed, navigating the field list with tabs doesn't show the info icon

I actually think that is ok. When purely focused, the info icon doesn't help much because it's based on a hover action. It would then just cause noise. I'm happy with it only appearing on hover.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
discover 265 +18 247
kibanaReact 294 +11 283
total +29

async chunks size

id value diff baseline
discover 436.4KB +4.8KB 431.6KB

page load bundle size

id value diff baseline
kibanaReact 642.1KB +12.7KB 629.4KB
lens 853.5KB -4.7KB 858.1KB
total +8.0KB

History

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

@andreadelrio andreadelrio merged commit 245c0a3 into elastic:master Aug 18, 2020
@kertal kertal mentioned this pull request Aug 19, 2020
Merged
kertal pushed a commit to kertal/kibana that referenced this pull request Aug 19, 2020
@andreadelrio @flash1293 @kertal
[Discover] Create field_button and add popovers to sidebar (elastic#7…
f2b1ec1 
…3226)

Co-authored-by: Michail Yasonik <michail.yasonik@elastic.co>
Co-authored-by: cchaos <caroline.horn@elastic.co>
Co-authored-by: Joe Reuter <johannes.reuter@elastic.co>
kertal added a commit that referenced this pull request Aug 19, 2020
@kertal @andreadelrio @flash1293
[Discover] Create field_button and add popovers to sidebar (#73226) (#…
4cbf55c 
…75389)

Co-authored-by: Andrea Del Rio <delrio.andre@gmail.com>
Co-authored-by: Michail Yasonik <michail.yasonik@elastic.co>
Co-authored-by: cchaos <caroline.horn@elastic.co>
Co-authored-by: Joe Reuter <johannes.reuter@elastic.co>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Aug 19, 2020
@gmmorris
Merge branch 'master' into task-manager/poller-mon
b0f7c0a 
* master: (112 commits)
  [Ingest Manager] Fix agent config rollout rate limit to use constants (elastic#75364)
  Update Node.js to version 10.22.0 (elastic#75254)
  [ML] Anomaly Explorer / Single Metric Viewer: Fix error reporting for annotations. (elastic#74953)
  [Discover] Fix histogram cloud tests (elastic#75268)
  Uiactions to navigate to visualize or maps (elastic#74121)
  Use prefix search invis editor field/agg combo box (elastic#75290)
  Fix docs in trigger alerting UI (elastic#75363)
  [SIEM] Fixes search bar Cypress test (elastic#74833)
  Add libnss3.so to Dockerfile template (reporting) (elastic#75370)
  [Discover] Create field_button and add popovers to sidebar (elastic#73226)
  [Reporting] Network Policy: Do not throw from the intercept handler (elastic#75105)
  [Reporting] Increase capture.timeouts.openUrl to 1 minute (elastic#75207)
  Allow routes to specify the idle socket timeout in addition to the payload timeout (elastic#73730)
  [src/dev/build] remove node-version from snapshots (elastic#75303)
  [ENDPOINT] Reintroduced tabs to endpoint management and migrated pages to use common security components (elastic#74886)
  [Canvas] Remove dependency on legacy expressions APIs (elastic#74885)
  Skip failing test in CI (elastic#75266)
  [Task Manager] time out work when it overruns in poller (elastic#74980)
  [Drilldowns] misc improvements & fixes (elastic#75276)
  Small README note on bumping memory for builds (elastic#75247)
  ...
@kertal kertal mentioned this pull request Aug 31, 2020
Merged
1 task
kertal added a commit that referenced this pull request Sep 1, 2020
@kertal
[Discover] Add sidebar jest test (#76286)
27bdc88 
* Add Jest test removed in #73226
kertal added a commit to kertal/kibana that referenced this pull request Sep 1, 2020
@kertal
[Discover] Add sidebar jest test (elastic#76286)
d561245 
* Add Jest test removed in elastic#73226
kertal added a commit that referenced this pull request Sep 1, 2020
@kertal
[Discover] Add sidebar jest test (#76286) (#76370)
1ba225f 
* Add Jest test removed in #73226
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@myasonik myasonik myasonik left review comments

@flash1293 flash1293 flash1293 approved these changes

@wylieconlon wylieconlon wylieconlon left review comments

@lizozom lizozom lizozom approved these changes

@cchaos cchaos cchaos approved these changes

@kertal kertal kertal approved these changes

Assignees
No one assigned
Labels
Feature:Discover Discover Application Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.10.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@andreadelrio @elasticmachine @kertal @cchaos @kibanamachine @wylieconlon @flash1293 @lizozom @myasonik