-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[Discover] [Unified Tabs] Tabs new design polishes #244179
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
base: main
Are you sure you want to change the base?
[Discover] [Unified Tabs] Tabs new design polishes #244179
Conversation
|
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
…2/kibana into discover-tabs-add-separators
2cbfb69 to
6224ef6
Compare
| // dismisses action popover when the selected tab changes | ||
| useEffect(() => { | ||
| if (prevSelectedItemIdRef.current !== selectedItemId && !isSelected && isActionPopoverOpen) { | ||
| setActionPopover(false); | ||
| } | ||
| prevSelectedItemIdRef.current = selectedItemId; | ||
| }, [selectedItemId, isSelected, isActionPopoverOpen]); |
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.
We can also do it simpler, but violating useEffect deps rule, so I went with ref here. I'm open for discussion though
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.
It feels like there should be an easier way to do this, but I'm not sure off the top of my head tbh. So I think it's good for now 👍
davismcphee
left a comment
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 the updates and fixes! It looks a lot more polished with the separators now 👌
I noticed a few minor things while testing locally and threw together a PR against this branch attempting to address them: akowalska622#3. Would you mind giving it a try and merging if all seems good? I think with these final touches the new tabs styles will feel nice and polished 🙂
| // dismisses action popover when the selected tab changes | ||
| useEffect(() => { | ||
| if (prevSelectedItemIdRef.current !== selectedItemId && !isSelected && isActionPopoverOpen) { | ||
| setActionPopover(false); | ||
| } | ||
| prevSelectedItemIdRef.current = selectedItemId; | ||
| }, [selectedItemId, isSelected, isActionPopoverOpen]); |
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.
It feels like there should be an easier way to do this, but I'm not sure off the top of my head tbh. So I think it's good for now 👍
| rgb(255, 0, 0) ${euiTheme.size.l}, | ||
| rgb(255, 0, 0) calc(100% - ${euiTheme.size.l}), |
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.
Why did we change the size of the overflow shadow? It looks a bit big now imo.
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 catching, it's a leftover from testing and playing around with shadow, I'll revert
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.
Addressed in a9b652a
|
Thanks for the review and additional polished @davismcphee! I'll definitely take a look today and update my branch with your suggestions, thanks for your time and effort ❤️ |
MiloszRadzynski
left a comment
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 great work! 🔥🚀
|
/ci |
⏳ Build in-progress, with failures
Failed CI StepsTest Failures
History
|
Summary
Followup: #243452
Resolves: #244153
A-la-carte: https://akowalska622-pr-244179-discover-tabs-add-separators.kbndev.co/
Initially issue focused only on adding separators, however a few more requirements for small polishes/bugfixes were added.
The complete list with requirement can be found in the issue
Most of the commits (apart from small clean ups) are scoped to a single bug/improvement.
Separators
Tabs should have separators, as per design:

Requirements for separators:
+button)It couldn't be done with pure css (targeting sibling element), because of wrappers (draggable wrapper and preview wrapper) the tabs aren't direct siblings. I eventually settled for managing the visibility of the separator from within the parent (tabs bar), based on selected and hovered state of a tab and previous tab to the one which is being hovered/selected.
From my testing it was the cleanest and sturdiest approach, but I'm open to different ones as well.
PR changes:
Loading spinner should respect the tab border radius
Fixed overflowing shadow and layout shift on keyboard navigation
Drag and drop styling (transparent all tabs, but selected and dragged)
Dismissing tab action menu if switched to a different tab
Checklist
Check the PR satisfies following conditions.
Reviewers should verify this PR satisfies this list as well.
release_note:breakinglabel should be applied in these situations.release_note:*label is applied per the guidelinesbackport:*labels.Identify risks
Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss.
Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging.