Skip to content

Make LabelsInput accept container's size#53552

Merged
bl-nero merged 3 commits intomasterfrom
bl-nero/labels-input
Apr 1, 2025
Merged

Make LabelsInput accept container's size#53552
bl-nero merged 3 commits intomasterfrom
bl-nero/labels-input

Conversation

@bl-nero
Copy link
Contributor

@bl-nero bl-nero commented Mar 28, 2025

This helps sizing it properly in contexts like the role editor. Also: replace the trash can icon with a cross icon.

This PR changes the internal structure of LabelsInput from a bunch of boxes to a regular table, which is also more accessible.

Before:
Screenshot 2025-03-28 at 19 00 36

After:
Screenshot 2025-03-28 at 19 01 08

Notes:

  1. We still have two separate components for handling labels; unifying these is not in the scope of this PR.
  2. One of the issues with the current role editor UI is that the add label buttons are too big, but I'll solve it separately, with a PR spanning multiple components; having bigger button is less jarring than having inconsistent sizes.

Contributes to #52221
Contributes to #52222

This helps sizing it properly in contexts like the role editor.
@bl-nero bl-nero added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v16 backport/branch/v17 labels Mar 28, 2025
@bl-nero bl-nero marked this pull request as ready for review March 28, 2025 18:16
@github-actions github-actions bot requested review from gzdunek and ravicious March 28, 2025 18:16
${props => props.theme.typography.body3}
`;

const LabelTable = styled.table`
Copy link
Member

Choose a reason for hiding this comment

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

This PR changes the internal structure of LabelsInput from a bunch of boxes to a regular table, which is also more accessible.

Does it mean that tables are inherently more accessible in this scenario? I didn't know about this, but I wouldn't mind reading up on it, do you have any good resources on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, they are better. Though I don't have any actual reading material that I could recommend, let me jus prove my case with a demo of how Apple's own VoiceOver reads our UI. It would be even better to use a grid role for this, as this is an interactive table, but that would, by convention, require us to implement a different keyboard navigation scheme, so I didn't do it. Table, however, was a low-hanging fruit here, since I already needed to rebuild the layout anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the recording!

@bl-nero bl-nero enabled auto-merge April 1, 2025 11:22
@bl-nero bl-nero added this pull request to the merge queue Apr 1, 2025
Merged via the queue into master with commit 421fcc7 Apr 1, 2025
40 checks passed
@bl-nero bl-nero deleted the bl-nero/labels-input branch April 1, 2025 12:25
@backport-bot-workflows
Copy link
Contributor

@bl-nero See the table below for backport results.

Branch Result
branch/v16 Failed
branch/v17 Create PR

@bl-nero
Copy link
Contributor Author

bl-nero commented Apr 2, 2025

(Dropping v16 backport: too complex, not worth it.)

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

Labels

backport/branch/v17 no-changelog Indicates that a PR does not require a changelog entry size/sm ui

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments