Skip to content
This repository has been archived by the owner on Aug 13, 2023. It is now read-only.

Add prop to allow us to set aria-hidden on live label #3206

Merged
merged 4 commits into from
Mar 4, 2020

Conversation

tochwill
Copy link
Contributor

@tochwill tochwill commented Mar 4, 2020

Resolves bbc/simorgh#5749

Overall change:
Styled components strips out 'aria-hidden' when rendering on the server. Will look into this, but in the meantime we can pass a prop to the live label that lets us set this attribute.

Code changes:

  • Add prop to live label to set ariaHidden to true or false

  • I have assigned myself to this PR and the corresponding issues
  • Automated jest tests added (for new features) or updated (for existing features)
  • This PR requires manual testing

@tochwill tochwill added the ws-home Tasks for the WS Home Team label Mar 4, 2020
@tochwill tochwill self-assigned this Mar 4, 2020
Copy link
Contributor

@sareh sareh left a comment

Choose a reason for hiding this comment

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

Looks good. We will look at pulling out LiveLabel into its own Psammead component in another PR: #3203

Note for testing: since Storybook is running on the client only, this fix won't seem necessary, until the component is pulled into an application that is server-rendered.

Copy link
Contributor

@DenisHdz DenisHdz left a comment

Choose a reason for hiding this comment

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

Just a suggestion to destructure the ariaHidden as we do with service and script.
Also, can you update the PR description, please?

@@ -227,7 +227,9 @@ export const Link = styled.a`
}
`;

export const LiveLabel = styled.span`
export const LiveLabel = styled.span.attrs(
props => props.ariaHidden && { 'aria-hidden': 'true' },
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
props => props.ariaHidden && { 'aria-hidden': 'true' },
${({ ariaHidden }) => ariaHidden && { 'aria-hidden': 'true' }},

Copy link
Contributor

@DenisHdz DenisHdz left a comment

Choose a reason for hiding this comment

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

LGTM

@tochwill tochwill merged commit 6a5780b into latest Mar 4, 2020
@tochwill tochwill deleted the update-live-aria-hidden branch March 4, 2020 09:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ws-home Tasks for the WS Home Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show Turkish & Vietnamese translation for Live label
3 participants