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(datatable): set overflowMenuOnHover to false #15909

Merged

Conversation

mranjana
Copy link
Contributor

@mranjana mranjana commented Mar 7, 2024

Closes #15480

DataTable overflowMenuOnHover set to false default

Changelog

Changed

  • changed overflowMenuOnHover to false

Testing / Reviewing

{{ DataTable overflowMenu}}

@mranjana mranjana requested a review from a team as a code owner March 7, 2024 09:20
Copy link
Contributor

github-actions bot commented Mar 7, 2024

DCO Assistant Lite bot All contributors have signed the DCO.

Copy link

netlify bot commented Mar 7, 2024

Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 88d06a0
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/65f3dd465196010009fc652d
😎 Deploy Preview https://deploy-preview-15909--v11-carbon-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 configuration.

@mranjana
Copy link
Contributor Author

mranjana commented Mar 7, 2024

DCO Assistant Lite bot: Thanks for your submission! We ask that you sign our Developer Certificate of Origin before we can accept your contribution. You can sign the DCO by adding a comment below using this text:

I have read the DCO document and I hereby sign the DCO.

You can retrigger this bot by commenting recheck in this Pull Request

I have read the DCO document and I hereby sign the DCO.

@mranjana
Copy link
Contributor Author

mranjana commented Mar 7, 2024

recheck

@mranjana
Copy link
Contributor Author

mranjana commented Mar 7, 2024

I have read the DCO document and I hereby sign the DCO.

Copy link
Member

@alisonjoseph alisonjoseph left a comment

Choose a reason for hiding this comment

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

I believe this change should only be made in the example in storybook not in the actual component. (this would be a breaking change otherwise)
https://github.com/carbon-design-system/carbon/blob/main/packages/react/src/components/DataTable/stories/DataTable-toolbar.stories.js#L227

@mranjana mranjana requested a review from alisonjoseph March 14, 2024 13:59
Copy link
Member

@alisonjoseph alisonjoseph 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
Copy link
Collaborator

tw15egan commented Mar 14, 2024

@alisonjoseph, Based on the discussion, it seems like this is intentional #15480 (comment)

@tay1orjones is this still the desired outcome?

@alisonjoseph
Copy link
Member

alisonjoseph commented Mar 14, 2024

Seems like this change is correct based on @tay1orjones last comment?

After discussing further, the determination is this will just be changing overflowMenuOnHover to be false by default. It's already mentioned on the website to be this way, so this unifies the implementation against the current documentation. This isn't new design guidance either, it's been present for quite a while on the website.

@tw15egan
Copy link
Collaborator

@alisonjoseph, my understanding was that this is to unite the website documentation and the default value of the prop with the storybook. On the website we state:

By default, the overflow menu icons are persistent on each row. Having the overflow menus always visible signals to the user actions can be taken on the table rows. Alternatively, a product team may use the overflowMenuOnHover prop to only show the overflow menu on hover and focus to reduce the visual clutter of an overflow menu on every row.

So it sounds like we should set overflowMenuOnHover to false by default so that they are all persistent

@alisonjoseph
Copy link
Member

@tw15egan I think we are saying the same thing, this PR does set overflowMenuOnHover to false, which means it shows by default. The prop name is confusing.

@tw15egan
Copy link
Collaborator

tw15egan commented Mar 14, 2024

@alisonjoseph It sounded like we wanted to set the default value of this prop to false, not just the storybook story. The reason for that is @tay1orjones comments about this not being considered a breaking change:

We think this does not constitute a breaking change because:

  • This is a meaningful improvement to usability and accessibility for all users
  • It has a relatively low surface area and should not impact existing application test harnesses
  • overflowMenuOnHover remains configurable - consumers can set it to true if they prefer the old functionality

The original PR commit is the change I was expecting

@alisonjoseph
Copy link
Member

ohhh @tw15egan I completely misunderstood the original issue then.

@mranjana mranjana requested a review from alisonjoseph March 15, 2024 05:46
Copy link
Member

@alisonjoseph alisonjoseph left a comment

Choose a reason for hiding this comment

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

LGTM! SO sorry for the confusion with this. Thanks for the contribution!

Copy link
Collaborator

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

LGTM 👍 ✅

@tw15egan tw15egan added this pull request to the merge queue Mar 15, 2024
Merged via the queue into carbon-design-system:main with commit 72b2d51 Mar 15, 2024
20 checks passed
preetibansalui pushed a commit to tay1orjones/carbon that referenced this pull request Apr 24, 2024
…m#15909)

* fix(datatable): set overflowMenuOnHover to false

* fix(datatable): set overflowMenuOnHover to false in story

* fix(datatable): set overflowMenuOnHover to false
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.

[Dev] Data Table Implementation (Overflow menu)
3 participants