WORLDSERVICE-543: Refactor CTAs #12603
Conversation
| <RightChevron css={styles.chevron} /> | ||
| )} | ||
| <CallToActionLink to={linkAddress} css={styles.callToActionLink}> | ||
| <CallToActionLink.FlexWrapper> |
There was a problem hiding this comment.
Could the flexwrapper always be included as part of the component, so that we don't have to specifiy it here?
There was a problem hiding this comment.
Not all implementations need Flex. Catharine was keen to only use Flex when it's needed. It's currently used by the Message Banner and the Uploader Embed, (E.g. when the link is used to look a bit like a button, rather than an inline link).
Another option would be to use a Div with flex styles inside the Message Banner and the Uploader Embed. Then we could delete the FlexWrapper. Would this be better?
There was a problem hiding this comment.
Perhaps? I am keen to keep this component & its use as simple as possible. But why don't we get a second opinion because I'm not sure (and don't want you to go down the path of refactoring it if it's not recommended). Summoning @HarveyPeachey as our Component Standards expert)
There was a problem hiding this comment.
I think I prefer this separated flex wrapper. If we did the other option of creating a div with flex, you'll just be duplicating the code. And if we add a prop to render it conditionally we're starting to run into the whole prop drilling problem, even though it's only one level. Maybe we rename it to something like ButtonLikeWrapper, so it's a bit more explicit about what it's doing?
Feel free to get another opinion on this, as I've partly had influence by chatting to Izzy about the amendments to the current compound component standards
There was a problem hiding this comment.
Is there a specific accessibility reason behind not wanting to use flex in this case? I can understand if there was content that reflows differently between screen sizes, but since the chevron is a purely decorative and isn't read out by screenreaders anyways, I would have thought that it wouldn't matter as much.
There was a problem hiding this comment.
I am happy to leave the flex wrapper as is - for the reasons stated above. Plus, if you're trying to push through some amendments to the compond component standards, then it's OK to influence what we do here, as we want to have a good example of what it should look like - this is the best way to make progress towards the standards we want (evolutionary architecture FTW!)
There was a problem hiding this comment.
Based on this thread:
- I have renamed FlexWrapper to ButtonLikeWrapper and kept this as part of the component
On your point @hotinglok, there is a discussion of Flex on this PR.
As I understand it, the issue is that flex is not needed for this component. Whilst it may not have a negative impact on users - it's better from an A11y stand point to keep styles and html as minimal as possible. You can see the A11y specs here for reference: AACs, Screen reader UX
Furthermore, since the CallToAction link wasn't built to be used without Flex, there was a lot of unnecessary HTML coming through.
Previously
<a href="">
<div>
<span>
<span>Site yoroheje</span>
<svg><path></path></svg>
</span>
</div>
</a>
Now
<a href="">
<span>Data saving version</span>
<svg><path></path></svg>
</a>
So it wasn't a requirement to do this for A11y. We saw the opportunity to make the CallToAction Link component more reusable.
| css({ | ||
| display: 'inline-block', | ||
| padding: `${pixelsToRem(13)}rem 0 ${pixelsToRem(13)}rem`, |
There was a problem hiding this comment.
| padding: `${pixelsToRem(13)}rem 0 ${pixelsToRem(13)}rem`, | |
| padding: `${pixelsToRem(12)}rem 0 ${pixelsToRem(12)}rem`, |
I think this was originally 12px looking at https://www.test.bbc.com/pidgin/articles/c7k3zrd3536o. Was this done intentionally? Just checking!
There was a problem hiding this comment.
Yes it's a weird one - I lost a pixel when I removed the outer div. I needed to add 26 pixels total to make the 44 pixel touch target. It was 24 previously
There is an argument to use a nicer round number and make the touch target larger - but I made the call to use 13pixels so I didn't change the current experience.
| <RightChevron css={styles.chevron} /> | ||
| )} | ||
| <CallToActionLink to={linkAddress} css={styles.callToActionLink}> | ||
| <CallToActionLink.FlexWrapper> |
There was a problem hiding this comment.
Is there a specific accessibility reason behind not wanting to use flex in this case? I can understand if there was content that reflows differently between screen sizes, but since the chevron is a purely decorative and isn't read out by screenreaders anyways, I would have thought that it wouldn't matter as much.
| <CallToActionLink.Text size="brevier" fontVariant="sansBold"> | ||
| {toMainSite} | ||
| </CallToActionLink.Text> | ||
| <CallToActionLink.Chevron size="brevier" /> | ||
| </CallToActionLink> |
There was a problem hiding this comment.
Should the bottom of the chevron be aligned with the bottom of the text with this if the link doesn't have an underline? It isn't on live either, but just wondering.
There was a problem hiding this comment.
Do you have a screen grab which shows what you mean?
There was a problem hiding this comment.
So, the uploader test page conveniently has one CTA link with an underline, one without (by default without hovering). Ignore my original comment, it doesn't really make sense since I looked again and realised that the underline isn't aligned with the bottom of the chevron at all. There is still an inconsistency between the two though about how far below the underline appears beneath the text.
In this first screenshot below, the underline appears slightly lower than the bottom of the chevron:
Here the uploader form itself, the underline appears closer to the text and doesn't appear below or aligned with the bottom of the chevron:
There was a problem hiding this comment.
Looking at the designs*
For components which have a "button like" background (Message Banner and Uploader), the Chevron is aligned in the vertical middle of the "button like" background.
For components that appear inline (LiteSiteCTA). We opted for aligning it with the middle of the text. Here are the designs.
*I can't access the designs for the message banner or uploader component (they're on Zeplin - which is an old UX tool). But this is how it appears to me to have been designed.
There was a problem hiding this comment.
@hotinglok I wonder if the difference is because of the 'g' in 'Data saving version' - the underline has to move down to cater for that letter. In the example of Send form, there are no characters which go below the line
| <CallToActionLink.Text size="brevier" fontVariant="sansBold"> | ||
| {toMainSite} | ||
| </CallToActionLink.Text> | ||
| <CallToActionLink.Chevron size="brevier" /> | ||
| </CallToActionLink> |
There was a problem hiding this comment.
@hotinglok I wonder if the difference is because of the 'g' in 'Data saving version' - the underline has to move down to cater for that letter. In the example of Send form, there are no characters which go below the line
src/app/components/CallToActionLinkComponent/CallToActionLinkContext.tsx
Outdated
Show resolved
Hide resolved
Resolves JIRA: https://jira.dev.bbc.co.uk/browse/WORLDSERVICE-543
Summary
Refactors 3 CTA components into 1.
Code changes
-- Message Banner
-- Uploader Embed
-- Korean Download Page
-- Article Headline (Canonical To Lite Site CTA)
-- LiteSiteCTA (Banner at top of Lite Site)
Developer Checklist
Testing
Ready-For-Test, Local)Ready-For-Test, Test)Ready-For-Test, Preview)Ready-For-Test, Live)Additional Testing Steps
-- Message Banner - https://www.bbc.com/kyrgyz
-- Uploader Embed - https://www.test.bbc.com/pidgin/articles/c7k3zrd3536o
-- Korean Download Page - https://www.bbc.com/korean/downloads
-- Article Headline (Canonical To Lite Site CTA) - (note due to an experiment the copy may vary) - https://www.bbc.com/gahuza/articles/c8xpj9vnd5wo
-- LiteSiteCTA (Banner at top of Lite Site) - https://www.bbc.com/gahuza.lite
Find historic A11y specs on the Jira ticket.
Useful Links