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

ActionMenu text reflow fix #700

Merged
merged 4 commits into from
Aug 22, 2024
Merged

ActionMenu text reflow fix #700

merged 4 commits into from
Aug 22, 2024

Conversation

rezrah
Copy link
Collaborator

@rezrah rezrah commented Aug 16, 2024

Summary

Resolves https://github.com/github/primer/issues/3758 (SEV-1 ⚠️ )

Removes truncation of long text in ActionMenu button and lists, allowing it to instead reflow per WCAG guidelines.

List of notable changes:

  • Removed fixed height, text truncation and ellipsis on buttons to allow text to flow onto multiple lines if needed
  • Fixed issues with inherited paddings, which weren't previously accurate
  • Removed truncation and ellipsis on popup list items
  • Widths on action list items are now dynamically calculated

Steps to test:

  1. Use this visual diff to see the before/after of this change. The story name will change after reviews are in, because the diff won't work if that's done first.

Supporting resources (related issues, external links, etc):

Contributor checklist:

  • All new and existing CI checks pass
  • Tests prove that the feature works and covers both happy and unhappy paths
  • Any drop in coverage, breaking changes or regressions have been documented above
  • New visual snapshots have been generated / updated for any UI changes
  • All developer debugging and non-functional logging has been removed
  • Related issues have been referenced in the PR description

Reviewer checklist:

  • Check that pull request and proposed changes adhere to our contribution guidelines and code of conduct
  • Check that tests prove the feature works and covers both happy and unhappy paths
  • Check that there aren't other open Pull Requests for the same update/change

Screenshots:

Please try to provide before and after screenshots or videos

Before After

Screenshot 2024-08-16 at 17 35 16

Screenshot 2024-08-16 at 17 35 00

Copy link

changeset-bot bot commented Aug 16, 2024

🦋 Changeset detected

Latest commit: aad7718

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@primer/react-brand Patch
@primer/brand-primitives Patch
@primer/brand-e2e Patch
@primer/brand-fonts Patch
@primer/brand-config Patch
@primer/brand-storybook Patch

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

Copy link
Contributor

github-actions bot commented Aug 16, 2024

🟢 No design token changes found

@rezrah rezrah changed the title a11y fix to action menu, allowing text to reflow instead of truncate ActionMenu reflow fix Aug 16, 2024
@rezrah rezrah changed the title ActionMenu reflow fix ActionMenu text reflow fix Aug 16, 2024
Copy link
Contributor

github-actions bot commented Aug 16, 2024

⚠️ Visual differences found

Our visual comparison tests found UI differences.

Please review the differences by using the test artifacts to ensure that the changes were intentional.

Artifacts can be downloaded and reviewed locally.

Download links are available at the bottom of the workflow summary screen.

Example:

artifacts section of workflow run

If the changes are expected, please run npm run test:visual:update-snapshots to replace the previous fixtures.

Review visual differences

Copy link
Contributor

@joshfarrant joshfarrant left a comment

Choose a reason for hiding this comment

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

LGTM!

There's the one remaining TODO, but I'm guessing that's just there to make the before/after visual diff possible and will be fixed before merge? 🙂

Copy link

@simmonsjenna simmonsjenna left a comment

Choose a reason for hiding this comment

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

@rezrah Can we have the text within Select be left-aligned instead of center-aligned?

@rezrah
Copy link
Collaborator Author

rezrah commented Aug 20, 2024

@rezrah Can we have the text within Select be left-aligned instead of center-aligned?

@simmonsjenna 👋 Are you referring to the text inside the button or the popup menu?

If button, this is how it would look:

Screenshot 2024-08-20 at 09 49 59

If menu items, those are already left aligned and have are offset by default to make room for the selection arrow. This helps mitigate layout shift.

@simmonsjenna
Copy link

@rezrah Can we have the text within Select be left-aligned instead of center-aligned?

@simmonsjenna 👋 Are you referring to the text inside the button or the popup menu?

If button, this is how it would look:

Screenshot 2024-08-20 at 09 49 59

Yes! Inside the button itself as in the example you provided above.

@danielguillan
Copy link
Contributor

@simmonsjenna I understand your proposal to left-align the label but I believe keeping it centered is a more consistent option for a couple of reasons:

  1. Custom widths: if users set a custom width for the button, a left-aligned label might look awkward, especially if the label is much shorter than the button.
  2. Right-aligned action menu: when the control is right-aligned in a UI, a left-aligned label could apper misaligned and visually unbalanced.

I think we cannot easily test those two scenarios and switch from left to center alignment accordingly so the best option might be to keep the default centered alignment.

@simmonsjenna
Copy link

@danielguillan Thanks for the callout — I dug into it a bit more to understand how things are currently structured.

I didn't realize that the downward chevron was right next to the Button copy:
Screenshot 2024-08-21 at 6 46 42 AM

I was expecting it to be more like this with justify-content: space-between applied:
Screenshot 2024-08-21 at 6 46 30 AM

Screenshot 2024-08-21 at 6 50 36 AM

We can keep it as center aligned based on the variations that could occur 👍

@rezrah rezrah merged commit 47908f1 into main Aug 22, 2024
18 checks passed
@rezrah rezrah deleted the rezrah/action-menu-a11y-reflow branch August 22, 2024 14:14
@primer-css primer-css mentioned this pull request Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants