Add tabIndex to Viewer Link #695
Conversation
daniel-heppner-ibigroup
assigned amy-corson-ibigroup and unassigned daniel-heppner-ibigroup
| @@ -85,7 +85,7 @@ export const AnchorButton = styled.a` | |||
| } | |||
| `; | |||
|
|
|||
| export const LinkButton = styled.a` | |||
| export const StyledLink = styled.a` | |||
There was a problem hiding this comment.
🚨🚨 Breaking changes...
There was a problem hiding this comment.
I see what is going on. onClick on an anchor <a> element is incorrect. To use onClick, you have to use a button. The correct use with anchors is href.
There was a problem hiding this comment.
This is what I would do as a fix to avoid a breaking change:
- Revert the name changes
- Change to
export const LinkButton = styled(TransparentButton)
|
@binh-dam-ibigroup We changed these to links earlier this year due to some accessibility feedback. Because they behave as links (take a user to a different URL) it causes confusion to screenreaders when they're marked up as buttons. What qualifies this as a breaking change? If the lack of href is causing an issue, maybe we can do something like this? |
You renamed an exported symbol. |
|
How does |
|
@binh-dam-ibigroup Gotcha, I didn't realize those were getting exported in the I think the |
| @@ -28,7 +28,7 @@ class ViewTripButton extends Component<Props> { | |||
|
|
|||
| render(): ReactElement { | |||
| return ( | |||
| <S.ViewerButton onClick={this.onClick} type="button"> | |||
| <S.ViewerButton href="#" onClick={this.onClick} tabIndex={0}> | |||
There was a problem hiding this comment.
href="#" does not work, it will clear out the query params in the url bar and that will mess up OTP-RR state, so i guess if we must use <a>, we are stuck with not setting href and keep tabIndex={0}.
There was a problem hiding this comment.
@amy-corson-ibigroup Do this ^^
|
@amy-corson-ibigroup In the same vein as our team discussion using anchors the other day, one way to make the code correct (via a breaking change) is to make the ItineraryBody component accept some kind of URL template or URL getter function, so that the the stop viewer and trip viewer links actually work as links with |
There was a problem hiding this comment.
Sorry, I just noticed this playing with the keyboard. You need to implement an event handler for onKeyDown and respond to the ENTER key. Otherwise, the stop viewer and trip viewer links are inoperable when using the keyboard (in Storybook, you will see that no action is triggered when pressing ENTER while the link is focused). This, or inform the client that buttons are a much better solution.
|
Okay, I have reverted back to |
Changing the stop and trip viewer from buttons to links caused a slight regression where the links were not keyboard-focusable. This adds a
tabIndexto those links so that they appear back in the focus order. It also renames some of the involved styled components to reflect the fact that they are links and not buttons.