Add TrailingAction support to NavList #4697
Conversation
🦋 Changeset detectedLatest commit: 27e01a8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
|
|
||
| export type NavListTrailingActionProps = ActionListTrailingActionProps | ||
|
|
||
| const TrailingAction = ActionList.TrailingAction |
There was a problem hiding this comment.
I noticed when using TrailingAction and there's already an "expand" button/icon, the styling is a bit off. I'm wondering if we'd want to disable TrailingAction in these instances, or allow them?
There was a problem hiding this comment.
You're right! I think we should disallow it.
There was a problem hiding this comment.
This has been addressed in this diff.
Let me know what you think of this approach!
I've supplemented this with:
- Storybook example to enable visual regression tests
- unit tests
| @@ -246,4 +247,26 @@ export const WithGroup = () => ( | |||
| </PageLayout> | |||
| ) | |||
|
|
|||
| export const WithTrailingAction = () => { | |||
There was a problem hiding this comment.
Non-blocking: A story showing this feature within sub items could be cool!
There was a problem hiding this comment.
Done!
|
The failing test is a snapshot test which seems to be failing unrelated to my changes. |
Co-authored-by: Tyler Jones <tylerjdev@github.com>
khiga8
force-pushed
the
add-trailing-action-to-nav-list
branch
from
2a0a687
to
646110e
Compare
|
The test failure seems unrelated to my changes. It looks like changes in ID in the jest snapshot tests 🤔 . Any suggestions for moving forward here @primer/react-reviewers? |
| ) | ||
| } | ||
|
|
||
| export const WithBadExampleOfSubNavAndTrailingAction = () => { |
There was a problem hiding this comment.
Is this test just to show a bad implementation of NavList? If so, I'm wondering if we should add this under "DevOnly" story as I think this is what it's for 🤔 (e.g. "DevOnly" story)
.changeset/tender-parrots-refuse.md
Outdated
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "@primer/react": patch | |||
There was a problem hiding this comment.
I'm thinking this might be a minor, but will leave it up to you!
Co-authored-by: Tyler Jones <tylerjdev@github.com>
Fixes: https://github.com/github/primer/issues/1966
Unblocks: https://github.com/github/accessibility-audits/issues/2942
Changelog
New
This PR adds TrailingAction support to NavList, extending the support we added to ActionList in #4634.
Rollout strategy
Testing & Reviewing
Merge checklist