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

chore(data-table): update overflow menu storybook #11247

Merged
merged 5 commits into from
Apr 22, 2022
Merged

chore(data-table): update overflow menu storybook #11247

merged 5 commits into from
Apr 22, 2022

Conversation

aledavila
Copy link
Contributor

Closes #10747

Overflow menu wasn't hiding.

Changelog

Changed

  • fixed classname in stories

Testing / Reviewing

Check data table overflow menu

@aledavila aledavila requested a review from a team as a code owner April 19, 2022 04:23
@aledavila aledavila requested review from jnm2377 and dakahn April 19, 2022 04:23
@netlify
Copy link

netlify bot commented Apr 19, 2022

Deploy Preview for carbon-components-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 1be326c
🔍 Latest deploy log https://app.netlify.com/sites/carbon-components-react/deploys/62632490230f960009bddf58
😎 Deploy Preview https://deploy-preview-11247--carbon-components-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@aledavila aledavila requested review from tay1orjones and removed request for dakahn April 19, 2022 04:33
@netlify
Copy link

netlify bot commented Apr 19, 2022

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit 1be326c
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/626324906576cf0008a39085
😎 Deploy Preview https://deploy-preview-11247--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@aledavila
Copy link
Contributor Author

@jnm2377 & @tay1orjones I wanted your take on this if possible. So the issue as it stands is that the overflow menu wasn't hiding when overflowMenuOnHover is true. There are 2 places where that exists I'm not even sure if the prop in Table works. The other is in DataTable component I think that is the one that works. (we can look into that another time)

So to my issue as you see in my change it wasn't working because the prefix was incorrect. In order for this prop to work the user needs to pass in className="cds--table-column-menu" to the TableCell containing the overflow menu. We don't exactly surface this or tell people they need to pass it in unless theres other ideas on how to integrate it into our code(the css might be too intertwined to do that). Do you guys have any ideas on the best way to do that ? So its not exactly a bug just a lack of documentation.

@tay1orjones
Copy link
Member

@aledavila Based on #5822 and #5804, DataTable should be passing overflowMenuOnHover down to Table. Table is the only spot where the cds--data-table--visible-overflow-menu class is toggled on/off.

If overflowMenuOnHover requires placing cds--table-column-menu on a TableCell (and rendering an actual OverflowMenu), I agree we need to at the very least document it. A blurb in storybook docs could be enough.

On the bigger picture of how to integrate this more into our code, I wonder if the TableCell should have a containsOverflowMenu prop that toggles the cds--table-column-menu class so we avoid this problem in the future?

@jnm2377
Copy link
Contributor

jnm2377 commented Apr 19, 2022

@aledavila @tay1orjones I'm wondering if there is a different selector that can be used to toggle on/off the overflow menu so that users don't have to forcibly add cds--table-column-menu (since I'm assuming that classname is part of the selector that hides the overflow menu)?

@aledavila
Copy link
Contributor Author

@jnm2377 yes the way the styles were written it expects it to have that. And I thought about removing it but then other styles use that class

@jnm2377
Copy link
Contributor

jnm2377 commented Apr 19, 2022

I don't mean necessarily removing it from the stylesheet, but adding a selector that will support the use case of a column w/o that class so that it'll still work if a user doesn't add that class.

@tay1orjones
Copy link
Member

Would it work if the selectors using .#{$prefix}--table-column-menu were reconfigured to instead apply the styles if a table cell has an overflow menu as a descendant/child?

Something maybe like td:has(> .#{$prefix}--overflow-menu)

@tay1orjones
Copy link
Member

Bah, nope, that's a level 4 selector not available in browsers yet. Bummer.

We might not be able to get out of having to apply the class to the table cell.

@aledavila
Copy link
Contributor Author

@tay1orjones I've added the documentation. Let me know if that makes sense.

@kodiakhq kodiakhq bot merged commit f016281 into carbon-design-system:main Apr 22, 2022
kennylam pushed a commit to kennylam/carbon that referenced this pull request Jul 30, 2024
### Related Ticket(s)

Closes carbon-design-system#11142 

### Description

Add slug to modal

### Changelog

**New**

- add slug slot
- new modal with ai styles
- slug story in Experimental -> Slug -> Examples -> Modal

** note there is no composed modal yet for CWC

**Changed**

- bump packages
- checkbox markdown update

<!-- React and Web Component deploy previews are enabled by default. -->
<!-- To enable additional available deploy previews, apply the following -->
<!-- labels for the corresponding package: -->
<!-- *** "test: e2e": Codesandbox examples and e2e integration tests -->
<!-- *** "package: services": Services -->
<!-- *** "package: utilities": Utilities -->
<!-- *** "RTL": React / Web Components (RTL) -->
<!-- *** "feature flag": React / Web Components (experimental) -->
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.

[Bug]: DataTable overflowMenuOnHover is broken
3 participants