brand icon indicating copy to clipboard operation
brand copied to clipboard

ActionMenu text reflow fix

Open rezrah opened this issue 1 year ago • 3 comments

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):

  • https://github.com/github/primer/issues/3758

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

rezrah avatar Aug 16 '24 16:08 rezrah

🦋 Changeset detected

Latest commit: aad7718cb914f638b4dea34445743bd7cfebe1e2

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

changeset-bot[bot] avatar Aug 16 '24 16:08 changeset-bot[bot]

🟢 No design token changes found

github-actions[bot] avatar Aug 16 '24 16:08 github-actions[bot]

⚠️ 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

github-actions[bot] avatar Aug 16 '24 16:08 github-actions[bot]

@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.

rezrah avatar Aug 20 '24 08:08 rezrah

@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.

simmonsjenna avatar Aug 20 '24 15:08 simmonsjenna

@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.

danielguillan avatar Aug 21 '24 08:08 danielguillan

@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 👍

simmonsjenna avatar Aug 21 '24 14:08 simmonsjenna