-
Notifications
You must be signed in to change notification settings - Fork 221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WSTEAM1-989: Live Post Share Button #11693
Conversation
…1041-SPIKE-Web-Share-API
<div ref={viewRef}> | ||
<button type="button" ref={focusRef} onClick={handleShare} css={styles}> | ||
<ShareSvg /> | ||
<span>Share</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might need role text around these 2 spans to mitigate text splitting as much as we can
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
viewBox="0 0 16 16" | ||
fill="none" | ||
xmlns="http://www.w3.org/2000/svg" | ||
aria-hidden="true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing focusable false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
backgroundColor: 'transparent', | ||
cursor: 'pointer', | ||
'&:hover, &:focus-visible': { | ||
border: `${pixelsToRem(2)}rem solid ${palette.BRAND_BACKGROUND}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The focus outline doesn't work in Firefox (I see a dotted line), are we able to use our custom classes we already have for adding the focus indicator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't to do with this styling, looks like firefox has an override style for type=button. I think we'll need to add type='button' to the custom classes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or we can remove type button, don't think it's strictly needed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's needed as browsers would treat it as a submit button for a form https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/button-has-type.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Believe that is only if it's wrapped inside a form tag... but cool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently if you don't add it, it's possible that it can destroy the current state of the document https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#notes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done this now
span: { | ||
verticalAlign: 'middle', | ||
}, | ||
svg: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On hover or focus in Firefox when you change colour preferences (haven't checked WHC) you can't see the icon, are we able to fix this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you able to apply styling as per the 'submit' button on uploader, that has an outline that looks ok as far as I can tell?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay messing around with trial and error, I've managed to get the colour of text to apply to the border. We actually need to fix the border on the submit button, as currently it's set to black and won't adjust to the users colour preference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cna you raise a ticket for that please when you are done with this @HarveyPeachey
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes will do
@@ -7,7 +7,8 @@ const focusIndicator = ({ palette }: Theme) => css` | |||
// Adds focus indicator styling to all a and button elements by default. | |||
a:focus-visible, | |||
button:focus-visible, | |||
button[type='submit']:focus-visible { | |||
button[type='submit']:focus-visible, | |||
h3:focus-visible { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want this for all h3's across the site?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be fine as h3's aren't interactive elements, so they would only be able to be programmatically focused on https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/tabindex#:~:text=Some%20focusable%20HTML,focus%20navigation%20order).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, was just checking that you meant to do this for all h3s
@@ -7,7 +7,8 @@ const focusIndicator = ({ palette }: Theme) => css` | |||
// Adds focus indicator styling to all a and button elements by default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment out of date if adding h3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
backgroundColor: palette.BRAND_BACKGROUND, | ||
path: { | ||
fill: palette.WHITE, | ||
[mq.HIGH_CONTRAST]: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is 'mq.HIGH_CONTRAST' forced-colours? https://developer.mozilla.org/en-US/docs/Web/CSS/@media/forced-colors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is
export const HIGH_CONTRAST = `@media screen and (forced-colors: active)`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. Would be good if we could rename HIGH_CONTRAST to FORCE_COLOURS across the code base...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks very much @HarveyPeachey left a couple of comments
…/bbc/simorgh into WSTEAM1-1041-SPIKE-Web-Share-API
Resolves JIRA https://jira.dev.bbc.co.uk/browse/WSTEAM1-989
Overall changes
Adds a Share button for Live page posts.
Code changes
yarn dev-https
script to use HTTPS locally via Next.js, as share API is only available in secure contexts, so testing on a local network with IP address will work.Testing
There are currently some a11y issues, which I haven't managed to find a fix for:
Helpful Links
Add Links to useful resources related to this PR if applicable.
Coding Standards
Repository use guidelines