-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Discover] Update layout for unified histogram #139446
[Discover] Update layout for unified histogram #139446
Conversation
dbb0f08
to
951f1d2
Compare
d6bb507
to
fa35e14
Compare
…esizing functionality to handle window resizing edge cases
…ayout and a resizable panels layout, and switch to fixed panels when in mobile
…prove testability and reusability
great thx!
For me this looks ok! |
|
||
// useIsWithinBreakpoints triggers state updates which cause act | ||
// issues and prevent our resize events from being fired correctly | ||
// https://github.com/enzymejs/enzyme/issues/2073 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx a lot for sharing and documenting!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 thx for inlining lots of explanations, this is very helpful! Tested locally with Chrome, Firefox, Safari. Thanks for taking care of the performance impact!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davismcphee Amazing work here Davis! Happy to see this become a reality. Just a comment, when I resize and I let go of the cursor, is it possible to go back to the initial state of the resize handler instead of staying with the blue bar on? Right now the only way to get rid of the blue bar is by doing an extra click outside and that feels a bit off.
@kertal when will we be able to get rid of the beta badge for Field Statistics? that tab would look a lot better without it.
Thx Andrea, this is a good question, if I remember I hink it was less a technical choice to add beta than a design choice ... so, are we there now in a post-beta world? Interested also in @ryankeairns opinion here. FYI @ninoslavmiskovic |
Design-wise I think that feature has been available long enough that we'd be ok to remove the badge. |
+1 what Andrea said; design has been in place for some time. I suppose it's ultimately a product decision at this point The only nitpick I have is whether should use small tabs as opposed to the the default size. The font seems a bit heavy, visually, relative to the rest of the content on the screen. Here's a sneak peek at the small size: |
ok, any objections removing beta of the field statistics dear @ninoslavmiskovic @ghudgins @qn895? |
I agree with @ryankeairns regarding using a lighter font for the tabs. Regarding removing the "beta" label. I am not aware of what the process is. I am used to look at performance, scalability, user feedback etc. before removing the beta. When I look at Fullstory on how many users clicked on "Field Statistics" in Discover past 90 days, then it is 8%. Before removing the beta - I would ask the team: Do we have enough user feedback? If that is a "no", then we should keep it and get some more. In my opinion getting user feedback is one of the main purposes running "Beta" programs, and "alpha". |
…rce blur the Discover layout resize button after a resize
@andreadelrio The resize button is now blurred after resizing with a mouse which gets rid of the focus styles. @ryankeairns I've updated the layout to use small tabs instead: Regarding the badge, I can make the change whenever we come to a decision, but it would be best if we could decide soon since the goal is to get this in before feature freeze on Sept 20. |
…-ref HEAD~1..HEAD --fix'
Pinging @elastic/fleet (Team:Fleet) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making those updates, LGTM.
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: cc @davismcphee |
Summary
This PR adds support for resizing the histogram in Discover, and moves the view mode tabs below the histogram in preparation for the unified histogram project.
A la carte build: https://davismcphee-pr-139446-enhancement-discover-unified-histogra.kbndev.co
Flaky test run for new functional test: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/1179
Resolves #138697.
Checklist
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n supportDocumentation was added for features that require explanation or tutorialsAny UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker listFor maintainers