Repository navigation
Conversation
b82da36 to
84b8a47
Compare
|
Tagging miles here @miles-grant-ibigroup for review |
miles-grant-ibigroup
left a comment
miles-grant-ibigroup
left a comment
There was a problem hiding this comment.
I'm a little weary of some of the performance implications of all these extra checks, as well as introducing react props into the CSS render. However, I'm not entirely sure what the desired outcome of this PR is. Maybe @amy-corson-ibigroup could help?
If there is no other way to be a11y friendly I guess we could live with the performance hit, but it's a measurable 20-50ms in my testing which I think deserves a closer look
| if (!suppressNearby) { | ||
| if (!positionUnavailable) { | ||
| // Add to the selection handler lookup (for use in onKeyDown) | ||
| pushToIndexedOptions(locationSelected, optionId); | ||
| } | ||
|
|
There was a problem hiding this comment.
Doesn't this have the byproduct of position warnings never appearing when suppressNearby is active?
There was a problem hiding this comment.
/**
- If true, do not show nearbyStops or current location as options
/
suppressNearby?: boolean;
/*
for the warnings there was only one that i was aware and it's still working
Yes I believe that would be true but i'm not sure how to get a storybook with suppressNearby and location unavaliable will take a look into that
miles-grant-ibigroup
assigned
nikkisato and
amy-corson-ibigroup and unassigned
miles-grant-ibigroup
|
Hello! Just flagging that I see this and I will review this week |
|
Hello, I'm really sorry for the delay! We've had devs on vacation, I had to take unexpected time off yesterday, and I am now at a client onsite, so I haven't given this the attention it deserves. It looks like we're applying 
Would you also elaborate on the bug you're seeing or the situation you're encountering that |
|
I want to step back and describe the two issues we were trying to fix, because I think my earlier approach blurred them together. You're right about aria-disabled: applying it when the menu still shows "location unavailable" isn't an accessibility win. Sighted users see that message; disabling the toggle can block screen reader users from the same info. I'll remove that. Two issues we were fixing (I blurred them before): #1 ) Toggle activates but nothing happens — aria-expanded stayed true with no selectable options. aria-expanded="false" when there are no selectable options → fixes #1 On suppressNearby: Not original #948 scope — came up because TriMet uses it. We found the selectable lookup still counted a suppressed current-location entry. aria-disabled only applies there when the menu is literally empty. Proposed changes:
|
|
@miles-grant-ibigroup @amy-corson-ibigroup |
miles-grant-ibigroup
left a comment
miles-grant-ibigroup
left a comment
There was a problem hiding this comment.
One quick comment, but otherwise I think things look good from my end! But I haven't checked through what @amy-corson-ibigroup mentioned
| const showMenuList = isStatic || menuItems.length > 0; | ||
| const listBoxControls = showMenuList ? listBoxId : undefined; | ||
|
|
||
| const statusAnnouncementText = statusMessages.filter(Boolean).join(". "); |
There was a problem hiding this comment.
This causes problems in some languages (especially RTL languages). However, I'd be ok to merge this as-is if we add a TODO on here and the other line where we manually concatenate a string!
This branch has not been deployed
3 participants
This is a follow up ticket to fix my mistake on #948
Amy made a comment that menuItemCount would always be === 1
#948 (comment)
thought my solution would fix this since it was on mac that we found this issue
But after testing I think this solution works
I also added a desktop story of suppressNearby since that what we are using on our side
i also copied the accessibility overrides for the storybook from desktop to mobile was getting some error
The goal was to make sure that there was
no regression
on location unavailable (it will still appear saying current location not available)