-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix(overflowmenu): added typing to align prop #19131
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(overflowmenu): added typing to align prop #19131
Conversation
Thanks for your submission! We ask that you all 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. 1 out of 2 committers have signed the DCO. |
I have read the DCO document and I hereby sign the DCO. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for v11-carbon-web-components ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for v11-carbon-react ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
2731b08
to
a9b073c
Compare
recheck |
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.
A couple comments but otherwise looks good to me. Thanks for taking this on!
Having align
alongside direction
and flipped
is probably going to lead to some confusion. Prop/type comments help though since they explicitly mention "tooltip". The new OverflowMenu implementation calls it tooltipAlignment
to avoid this confusion.
This being the old implementation though I don't think it's necessary to try and get a new tooltipAlignment
to work alongside the existing align
that some might be using. Plus it would add another layer of complexity having to iron out the types for all that.
I just want to make note of this, there's no action for you to take here.
packages/react/src/components/OverflowMenu/OverflowMenu.stories.js
Outdated
Show resolved
Hide resolved
* chore(overflowmenu): added todo waiting on consolidation of carbon-design-system#19081 * chore(overflowmenu-storybook): updated storybook align options to latest list
@jose-biescas The first commit a9b073c was made with a user/config that doesn't match your github.com profile, so the DCO bot will continually fail on this one. We can override it for this PR though, just something to be aware of next time. |
@tay1orjones Apologies for that, not sure what happened. Will make sure future commits are also verified/signed. Thank you! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #19131 +/- ##
==========================================
- Coverage 84.37% 84.36% -0.02%
==========================================
Files 388 388
Lines 14547 14550 +3
Branches 4774 4753 -21
==========================================
+ Hits 12274 12275 +1
- Misses 2113 2116 +3
+ Partials 160 159 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2fcb9f0
PR carbon-design-system#19131 added align manually but I changed it to inherit from IconButtonProps (rather than repeating the definition).
PR carbon-design-system#19131 added align manually but I changed it to inherit from IconButtonProps (rather than repeating the definition).
Closes #18957
Adds a type to the
align
prop inOverflowMenu
Changelog
New
align
propertyTesting / Reviewing
Go to the
Default
storybook forOverflowMenu
and thealign
property can be controlled via a dropdown with allowed prop values