-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Components: Enhance Notice actions to allow more props like disabled and onClick with url #74094
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
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
I wonder if some of this was intentional to keep UI more or less consistent. |
Actions were added 7 years ago by @aduth and @gziolo and I couldn't find any mention of any intentions there, except this for future enhancements:
As mentioned and explained in the linked issue, actions for those notices need to be versatile, it makes it hard to use them for real-world cases. |
gmjuhasz
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.
Coming from Jetpack I also did find this Notice component lacking this functionality before, we even had our own Notice component to battle this. I think these changes still keep it simple, but a lot more versatile.
|
Flaky tests detected in c71acc2. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/20552994612
|
Notice actions now accept an 'href' property in addition to 'url', allowing more flexible anchor rendering.
|
If you want to build a completely free UI, you can render it via the <Notice status="info">
<Flex justify="start" wrap>
<Button variant="primary">Push Me</Button>
<Button variant="primary" disabled>Push Me</Button>
<Button variant="primary" isBusy>Push Me</Button>
<Button variant="primary" size="small">Push Me</Button>
</Flex>
</Notice>I wonder if there is anything that can't be achieved without extending the actions property 🤔 |
Yes, we can do that but having the actual component fixed is probably better and has no harm. In future, I would expect the actions to be completely flexible by making them composable by directly rendering |
|
Left some thoughts in #74090 (comment) |
| actions?: Array< | ||
| Pick< NoticeAction, 'label' | 'url' | 'onClick' > & { | ||
| openInNewTab?: boolean; | ||
| } |
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.
Ugh I'm conflicted about this API divergence between Snackbar and Notice now. Snackbar added the feature through a openInNewTab prop, and swaps the Button for an ExternalLink in that case (#69905).
Actually I think that might be preferable to supporting rel and target directly, because one of the guidelines of the design system is to have consistent cues (visual and accessible) on links that open new tabs. ExternalLink has those cues, while Button does not.
The problem is that Snackbar only has to support variant="link" buttons, while Notice actions have a wider API surface that conflicts with ExternalLink, so the TypeScript/documentation complexity is probably not worth the trouble to make it work polymorphically.
This alone may be a good reason to support components directly in actions, for the @wordpress/ui version of Notice.
Sorry @manzoorwanijk, this aspect was a bit more complicated that expected. We can probably merge this PR faster if we punt this rel/target forwarding part out of scope 😅 (The type fix here for openInNewTab where you moved it out from Notice and into Snackbar is good 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.
The issue here was that openInNewTab was only defined in the type but not implemented. So, it was unused anyway.
So, I replaced it with target and rel to have better control.
As you rightly said, here we have to support different variants and thus it makes more sense to have it this way.
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.
The problem is that:
- We don't really want to allow tab-opening links without the visual/accessible cues like
ExternalLinkhas. - It's not good that
Snackbaractions andNoticeactions have different APIs to achieve the same thing.
We can solve these cleanly in a new API in @wordpress/ui, but it's hard to do this in @wordpress/components.
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.
So what do we want to do here? Maybe we can close this PR, live with what we have for now, and hope to resolve the issues in @wordpress/ui?
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's not good that Snackbar actions and Notice actions have different APIs to achieve the same thing.
Why not replicate then? It shouldn't be too difficult. Maybe something like the Cover Block cover/shared.js could work here?
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.
Like Marco said in #74090 (comment), "quick wins and fixes" are fine, but we don't want to introduce new features that add maintenance/migration cost to these soon-to-be-deprecated componentry.
The onClick and disabled are quick fixes, but openInNewTab turned out to be on the level of a "new feature" that does indeed add to the future cost. So I'd prefer we remove the openInNewTab feature from the scope of this PR, given that we can support it cleaner in the new @wordpress/ui package.
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.
openInNewTab was already defined there (in types) before this PR but was not implemented.
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.
Yes, it's messed up but let's leave it as is, unimplemented. I haven't looked into whether the types can be corrected easily so a Notice action won't accept an onInNewTab property, but that doesn't have to be done in this PR if it isn't simple.
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.
Done. Thanks.
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.
The types for Notice are now clean and straightforward.
@manzoorwanijk my conclusion reading through the whole convo is the following: limit the scope of this PR, by removing the open in new tab elements (alas, we missed |
Replaces the 'target' and 'rel' props with a boolean 'openInNewTab' prop for Notice action buttons, which, if true, forces to render the ExternalLink
|
I have updated the PR (along with the description) to reduce the scope to do only three things:
|
👍 Looking better to me, I would like to test a bit, but want to see what others think now. |
|
I'm afraid I can't enthusiastically greenlight the |
This reverts commit a96c415.
mirka
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.
Looks good, thanks for your patience here 🙏
Related to #74090
What?
Enhances the
Noticecomponent's action buttons to support additionalButtoncomponent features.Why?
Notice action buttons were missing common Button features like
disabled, etc. TheopenInNewTabprop existed in types but wasn't implemented, andonClickwas ignored whenurlwas provided.How?
NoticeActiontypes to allowdisabledandonClickalongsideurlopenInNewTabfrom the types.Testing Instructions
npm run storybook:devTesting Instructions for Keyboard
Screenshots