A11y Seat #4653
Open
A11y Seat #4653
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Deploying orbit with
|
| Latest commit: |
b26ffc7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c0957544.orbit.pages.dev |
| Branch Preview URL: | https://marco-a11y-seat.orbit.pages.dev |
|
Storybook staging is available at https://kiwicom-orbit-marco-a11y-seat.surge.sh Playroom staging is available at https://kiwicom-orbit-marco-a11y-seat.surge.sh/playroom |
|
Size Change: +86 B (+0.02%) Total Size: 463 kB
ℹ️ View Unchanged
|
mvidalgarcia
force-pushed
the
marco/a11y-seat
branch
4 times, most recently
from
3b6fa9f to
9cbf260
Compare
mvidalgarcia
force-pushed
the
marco/a11y-seat
branch
from
9cbf260 to
ca70d09
Compare
| true: action("onClick"), | ||
| false: undefined, | ||
| }, | ||
| defaultValue: true, |
There was a problem hiding this comment.
It seems that defaultValue is deprecated in favour of just setting value on args, let's remove it?
It's a shame we cannot set (default) value and it would be correctly displayed in the Controls but instead there is still that button :( There seems to be multiple issues created for this but none of them is acknowledged by SB team 😬
There was a problem hiding this comment.
Good catch, I'll remove it. It's not a big deal.
| {seatContent} | ||
| </button> | ||
| ) : ( | ||
| <div {...commonProps} aria-disabled> |
There was a problem hiding this comment.
Is it necessary to set aria-disabled here? Especially if this element has no role?
There was a problem hiding this comment.
Hmm I hesitated till the last moment about leaving it or not. But after some reading, I believe it's not needed and could be misleading for screen readers.
I wonder if we should have aria-selected 🤔
| aria-labelledby={`${[ariaLabelledBy, titleId, descrId].join(" ").trim()}` || undefined} | ||
| fill="none" | ||
| role="img" | ||
| aria-disabled={!clickable || undefined} |
There was a problem hiding this comment.
Same as for the div, is it necessary to add aria-disabled here? If this is already not-interactive element.
| viewBox={size === SIZE_OPTIONS.SMALL ? "0 0 32 36" : "0 0 46 46"} | ||
| aria-labelledby={`${[ariaLabelledBy, titleId, descrId].join(" ").trim()}` || undefined} | ||
| fill="none" | ||
| role="img" |
There was a problem hiding this comment.
Would it be valuable to add here aria-selected? 🤔
There was a problem hiding this comment.
hehe #4653 (comment)
Not sure if here or in the wrapper.. but since we have aria-labelledby here..
There was a problem hiding this comment.
whoops
The attribute aria-selected is not supported by the role img.
and also
The attribute aria-selected is not supported by the role button. This role is implicit on the element button.
😞
There was a problem hiding this comment.
I believe we can use role="option" on the svg element and then aria-selected is valid. Does this make sense to you?
There was a problem hiding this comment.
Can we use it? From the docs it seems it can be used only inside listbox 😞
But thinking about it more, if the seat is clickable, it will already be "selected" because it's button and in case of e.g. booking where they wrap it in Popover, there will be also button, so I guess it's enough?
There was a problem hiding this comment.
Can we use it? From the docs it seems it can be used only inside listbox 😞
That's true, chatGPT tricked me..
But thinking about it more, if the seat is clickable, it will already be "selected" because it's button and in case of e.g. booking where they wrap it in Popover, there will be also button, so I guess it's enough?
Not sure I understand. How do you mark a button as selected accessibility-wise?
There was a problem hiding this comment.
I think I got confused by The attribute aria-selected is not supported by the role button. This role is implicit on the element button.
sarkaaa
reviewed
Mar 11, 2025
| return ( | ||
| <Stack inline grow={false} spacing="50" direction="column" align="center"> | ||
| {clickable ? ( | ||
| <button {...commonProps} onClick={onClick} tabIndex={0} type="button"> |
There was a problem hiding this comment.
I think we can exclude tabIndex={0} as the button has this option by default.
There was a problem hiding this comment.
Whoops good catch, it was probably a leftover. And thanks so much for the docs pointer :)
I wonder why this wasn't caught by the linter as it's clearly wrong 😅
Useful to see how the component behaves both with and without the handler
If the Seat is available and has onClick set, renders it as an interactive button, otherwise renders it as a non-interactive div Closes FEPLT-2265
mvidalgarcia
force-pushed
the
marco/a11y-seat
branch
from
ca70d09 to
8b9f7f5
Compare
|
Changes pushed to a fixup commit to ease the re-review 😅 thanks! |
mvidalgarcia
force-pushed
the
marco/a11y-seat
branch
from
8b9f7f5 to
d5f350f
Compare
mvidalgarcia
force-pushed
the
marco/a11y-seat
branch
from
d5f350f to
b26ffc7
Compare
![callstackai[bot]](https://avatars.githubusercontent.com/in/1040204?s=60&v=4){kind=small}
LLM Analysis of PR ChangesPull Request Analysis - A11y SeatHere's an analysis of the provided Git diff for the "A11y Seat" pull request: 1. Summary of Changes: This pull request focuses on improving the accessibility and semantic correctness of the
2. Potential Issues or Bugs:
3. Suggestions for Improvements:
4. Security Concerns:
5. Code Quality Observations:
Overall Feedback: This is a well-executed pull request that effectively improves the accessibility and semantic correctness of the Recommendations:
After addressing these points, this pull request should be merged. It demonstrably improves the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
chore(Seat): add onClick prop control to story
Useful to see how the component behaves both with and without the handler
fix(Seat): display as div when non-clickable
If the Seat is available and has onClick set, renders it as an
interactive button, otherwise renders it as a non-interactive div
Closes FEPLT-2265
It can be tested with yalc on booking. I used this diff:
Diff
✨
Description by Callstackai
This PR introduces a new feature for the Seat component, allowing it to render as a clickable button or a non-interactive div based on the presence of an onClick handler. It also adds a title prop to enhance accessibility.
Diagrams of code changes
sequenceDiagram actor User participant Seat participant SeatContent participant Button participant Div User->>Seat: Interact with seat Seat->>Seat: Check if seat is available Seat->>Seat: Check if onClick exists alt Seat is available & onClick exists Seat->>Button: Render as button Button->>SeatContent: Render seat content SeatContent->>SeatContent: Show SVG with label Note over SeatContent: Display selection circle if selected User->>Button: Click seat Button-->>User: Trigger onClick action else Seat unavailable or no onClick Seat->>Div: Render as div Div->>SeatContent: Render seat content SeatContent->>SeatContent: Show SVG with label end alt Price exists & Not (selected & unavailable) Seat->>Seat: Display price endFiles Changed