Skip to content

fix: update action button to use grid area to position notification badge #8409

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.

Sign up for GitHub

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

Jump to bottom
Merged
yihuiliao merged 5 commits into from
Jun 27, 2025
Merged

fix: update action button to use grid area to position notification badge #8409

yihuiliao merged 5 commits into from
Jun 27, 2025

Conversation

@yihuiliao
Copy link
Member

@yihuiliao yihuiliao commented Jun 18, 2025 •
edited
Loading

Chromatic:
https://www.chromatic.com/build?appId=5f0dd5ad2b5fc10022a2e320&number=976

Some things to note:

  1. The notification badges have shifted a little bit but it's pretty minimal
  2. ToggleButton's truncate to fit inside the container. It seems that previously, the button would exceed the grid but now it truncates to fit. This change is okay

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

For Action button stories, use dev tools to add flex-direction: column on the flex container and set up a specific width. The action buttons should not stretch to fit the container.

Also within dev tools, set a width on the action buttons and it should expand accordingly. For the notification badge, they should still positioned correctly.

🧢 Your Project:

@rspbot
Copy link

rspbot commented Jun 18, 2025

Build successful! 🎉

@snowystinger
Copy link
Member

snowystinger commented Jun 18, 2025

Can you run chromatic on this change?

@yihuiliao
Copy link
Member Author

yihuiliao commented Jun 19, 2025

Yep, I'm gonna hold off on this PR actually after we get some more info from design. Might require an API update...

@yihuiliao yihuiliao changed the title fix: prevent s2 action button from expanding to fit container wip: prevent s2 action button from expanding to fit container Jun 23, 2025
yihuiliao
yihuiliao commented Jun 25, 2025
View reviewed changes
packages/@react-spectrum/s2/src/ActionButton.tsx Outdated
},
flexShrink: 0,
order: 0
gridArea: 'icon',
Copy link
Member Author

@yihuiliao yihuiliao Jun 25, 2025

Choose a reason for hiding this comment

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

I know the avatar isn't an icon but for the grid area, it effectively takes up the place of an icon

@rspbot
Copy link

rspbot commented Jun 25, 2025

Build successful! 🎉

snowystinger
snowystinger reviewed Jun 25, 2025
View reviewed changes
@rspbot
Copy link

rspbot commented Jun 26, 2025

Build successful! 🎉

@yihuiliao yihuiliao changed the title wip: prevent s2 action button from expanding to fit container fix: prevent s2 action button from expanding to fit container Jun 26, 2025
@yihuiliao yihuiliao changed the title fix: prevent s2 action button from expanding to fit container fix: update action button to use grid area to position notification badge Jun 26, 2025
reidbarber
reidbarber approved these changes Jun 27, 2025
View reviewed changes
Copy link
Member

@reidbarber reidbarber left a comment

Choose a reason for hiding this comment

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

LGTM

@yihuiliao yihuiliao added this pull request to the merge queue Jun 27, 2025
Merged via the queue into main with commit 02a90a1 Jun 27, 2025
35 checks passed
@yihuiliao yihuiliao deleted the s2-actionbutton-container branch June 27, 2025 16:47
yihuiliao added a commit that referenced this pull request Jul 15, 2025
@yihuiliao
Revert "fix: update action button to use grid area to position notifi…
b6e1a41 
…cation badge (#8409)"

This reverts commit 02a90a1.
github-merge-queue bot pushed a commit that referenced this pull request Jul 15, 2025
@yihuiliao
Revert "fix: update action button to use grid area to position notifi…
0cdf701 
…cation badge (#8409)" (#8552)

This reverts commit 02a90a1.
yihuiliao added a commit that referenced this pull request Dec 9, 2025
@yihuiliao
Revert "Revert "fix: update action button to use grid area to positio…
cc32097 
…n notification badge (#8409)" (#8552)"

This reverts commit 0cdf701.
github-merge-queue bot pushed a commit that referenced this pull request Dec 9, 2025
@yihuiliao
fix: update action button to use grid area to position notification b…
848fc8b 
…adge (#9303)

* Revert "Revert "fix: update action button to use grid area to position notification badge (#8409)" (#8552)"

This reverts commit 0cdf701.

* add chromatic story

* fix lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@snowystinger snowystinger snowystinger approved these changes

@reidbarber reidbarber reidbarber approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@yihuiliao @rspbot @snowystinger @reidbarber