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

fix(data-table): fix table with overflow menu bugs #5794

Merged
merged 5 commits into from
Apr 7, 2020

Conversation

tw15egan
Copy link
Member

@tw15egan tw15egan commented Apr 2, 2020

Closes #5792

Fixes an issue where negative margin was added to fix a react specific bug with overflow menu. Added in a more specific selector so it will only target the React implementation. Fixed a few other visual bugs when hovering over the overflow menu, see pics below:

Screen Shot 2020-04-02 at 12 07 11 PM

Screen Shot 2020-04-02 at 12 07 15 PM

Changelog

Changed

  • Increased specificity with negative margin override so it only affects React implementation
  • light overflow menu now uses the correct field-02 token
  • Changed hover styles so that a) the overflow icon would not disappear when hovering over a menu item and b) there was not a small bar with an incorrect color showing

Testing / Reviewing

Test out any Data Table with an Overflow menu, and make sure positioning is correct. Also, check out styles when hovering over an overflow menu, and hover styles when the overflow menu is opened.

Worth checking out that the normal overflow menu is working as expected as well.

@tw15egan tw15egan requested a review from a team as a code owner April 2, 2020 19:12
@ghost ghost requested review from aledavila and joshblack April 2, 2020 19:12
@tw15egan tw15egan requested review from a team and designertyler and removed request for a team April 2, 2020 19:12
@netlify
Copy link

netlify bot commented Apr 2, 2020

Deploy preview for carbon-elements ready!

Built with commit adefc93

https://deploy-preview-5794--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Apr 2, 2020

Deploy preview for carbon-components-react ready!

Built with commit adefc93

https://deploy-preview-5794--carbon-components-react.netlify.com

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

LGTM 👍 - Thanks @tw15egan!

@designertyler
Copy link
Contributor

Hey @tw15egan I'm still seeing that bar at the bottom of the overflow icon.

I think it's actually the drop shadow peeking up from behind. It's more obvious on the white and g10 themes.

image

@tw15egan
Copy link
Member Author

tw15egan commented Apr 6, 2020

@designertyler should be fixed now 👍

Copy link
Contributor

@aledavila aledavila left a comment

Choose a reason for hiding this comment

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

lgtm

@designertyler
Copy link
Contributor

I think it's looking better, but there's still a very very slight color shift happening. Taking the eyedropper to it shows that it's only a couple values away.

What could be causing it? Is it possible to fix?

image

@tw15egan
Copy link
Member Author

tw15egan commented Apr 7, 2020

@designertyler I'm guessing it is the drop shadow peeking out. If we increase the small pseudo ::after element that we are using to hide most of the drop shadow, it's going to look strange since it will be larger than the focus outline. Not sure this will be noticeable to anyone as-is

Copy link
Contributor

@designertyler designertyler left a comment

Choose a reason for hiding this comment

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

This subtle color shift is an improvement to the current component, a better tradeoff and will be less noticeable than the focus issue that trying to solve it would cause.

@tw15egan tw15egan merged commit 528addc into carbon-design-system:master Apr 7, 2020
@tw15egan tw15egan deleted the data-table-fixes branch April 28, 2021 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Overflow menu]: 3 dots are not centered
4 participants