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

Add guidance to Media player #2517

Merged
merged 52 commits into from
Nov 6, 2019
Merged

Add guidance to Media player #2517

merged 52 commits into from
Nov 6, 2019

Conversation

rhenshaw56
Copy link
Contributor

@rhenshaw56 rhenshaw56 commented Oct 29, 2019 •
edited by jamesdonoh
Loading

Resolves bbc/simorgh#3139

Overall change: Adds guidance to Media player placeholder

Code changes:

  • Created guidance component
  • 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
    Screen Shot 2019-10-29 at 16 25 37

@rhenshaw56 rhenshaw56 added ws-articles Tasks for the WS Articles Team articles-av-epic labels Oct 29, 2019
@rhenshaw56 rhenshaw56 self-assigned this Oct 29, 2019
@rhenshaw56 rhenshaw56 changed the title 3139 guidance Add guidance to Media player Oct 29, 2019
12
12 reviewed Oct 29, 2019
View reviewed changes
packages/components/psammead-media-player/src/Guidance/index.jsx Outdated
fill: currentColor;
`;

const GuidanceIcon = () => (
Copy link
Contributor

@12 12 Oct 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Would it make more sense to put this guidance icon in @bbc/psammead-assets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@12 yeah, definitely it would. but depends on how much we'd be reusing it. was planning to build everything here first and slowly separate them out

Copy link
Contributor

Choose a reason for hiding this comment

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

Any updates on what you've decided to do? I still think we should extract this out to psammead-assets personally...

@rhenshaw56 rhenshaw56 added a11y-swarm An a11y swarm (clarify dev or full team in the desc) needs to be carried out before moving to test ux To be reviewed by UX before merging labels Oct 29, 2019
@rhenshaw56 rhenshaw56 marked this pull request as ready for review October 30, 2019 06:58
12
12 suggested changes Oct 30, 2019
View reviewed changes
packages/components/psammead-media-player/src/Guidance/index.jsx Outdated Show resolved Hide resolved
packages/components/psammead-media-player/src/Guidance/index.jsx Outdated Show resolved Hide resolved
packages/components/psammead-media-player/src/Guidance/index.jsx Outdated

Guidance.propTypes = {
message: string,
className: string,
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't see where or why we would pass this prop?

packages/components/psammead-media-player/src/Guidance/index.jsx Outdated
fill: currentColor;
`;

const GuidanceIcon = () => (
Copy link
Contributor

Choose a reason for hiding this comment

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

Any updates on what you've decided to do? I still think we should extract this out to psammead-assets personally...

packages/components/psammead-media-player/src/Placeholder/__snapshots__/index.test.jsx.snap Outdated

.c2 {
font-family: ReithSans,Helvetica,Arial,sans-serif;
font-weight: 700;
Copy link
Contributor

Choose a reason for hiding this comment

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

The UX specification says that the guidance message should have a normal font weight. This suggests that the guidance message will have a weight of 700 (and it does in Storybook).

packages/components/psammead-media-player/src/Guidance/index.jsx Outdated Show resolved Hide resolved
packages/components/psammead-media-player/src/Guidance/index.jsx Outdated Show resolved Hide resolved
simonsinclair
simonsinclair suggested changes Oct 30, 2019
View reviewed changes
Copy link
Contributor

@simonsinclair simonsinclair left a comment

Choose a reason for hiding this comment

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

It's great to see this come together, @rhenshaw56. I've added a few comments below.

packages/components/psammead-media-player/src/Guidance/index.jsx Outdated Show resolved Hide resolved
packages/components/psammead-media-player/src/Guidance/index.jsx Outdated Show resolved Hide resolved
packages/components/psammead-media-player/src/Guidance/index.jsx Outdated Show resolved Hide resolved
packages/components/psammead-media-player/src/Guidance/index.jsx Outdated Show resolved Hide resolved
packages/components/psammead-media-player/src/Placeholder/index.jsx Show resolved Hide resolved
packages/components/psammead-media-player/src/index.jsx Outdated Show resolved Hide resolved
packages/components/psammead-media-player/src/index.stories.jsx Outdated Show resolved Hide resolved
@rhenshaw56 rhenshaw56 mentioned this pull request Oct 30, 2019
Closed
1 task
HarveyPeachey
HarveyPeachey approved these changes Nov 5, 2019
View reviewed changes
Copy link
Contributor

@HarveyPeachey HarveyPeachey left a comment

Choose a reason for hiding this comment

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

LGTM

dr3
dr3 approved these changes Nov 5, 2019
View reviewed changes
Copy link
Contributor

@dr3 dr3 left a comment

Choose a reason for hiding this comment

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

LGTM, can you please raise an issue to address the message not being fully readable on 240px wide (lowest we support)
image

12
12 reviewed Nov 5, 2019
View reviewed changes
packages/components/psammead-media-player/src/Placeholder/index.test.jsx Outdated
'Guidance: May contain strong language, sexual or violent content that may offend.',
...withDuration,
}}
className="foo"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we setting className here?

@jamesdonoh
Copy link
Contributor

Needs manual testing + dev a11y swarm

@rhenshaw56 rhenshaw56 mentioned this pull request Nov 5, 2019
Closed
1 task
@rhenshaw56
Copy link
Contributor Author

rhenshaw56 commented Nov 5, 2019 •
edited
Loading

Dev/A11y Swarm

-https://bbc.github.io/accessibility-news-and-you/accessibility-news-and-testers

  • Axe
  • Design
  • Colour Contrast
  • Colour blindness
  • Windows HCM - Darkened overlay of HCM blocks the placeholder image from being viewed.
  • FF Colour Preferences
  • Text 200%, Page 200%
  • Icons | need to add visually hidden text for guidance icon ?
  • [ ] Heading Order
  • Keyboard Navigation

Screen Readers (IE11) - Harvey:

  • ZoomText Magnifier/Reader - When tabbing with speech Guidance is not highlighted or read out. Only individual words are read out when clicking with the speakit tool
  • Dragon NaturallySpeaking
  • JAWS - Read out guidance then states it's clickable
  • Read&Write

Mac, iOS, and Android:

  • VoiceOver iOS (iPhone/iPad)
  • VoiceOver OS (Mac)
  • TalkBack (Chrome on Android)

This was referenced Nov 6, 2019
Closed
Closed
@PriyaKR PriyaKR self-assigned this Nov 6, 2019
@PriyaKR
Copy link
Contributor

PriyaKR commented Nov 6, 2019

Is the guidance just been added to canonical media player page or even on amp?

@rhenshaw56
Copy link
Contributor Author

Is the guidance just been added to canonical media player page or even on amp?

@PriyaKR this is just for the canonical media player

@PriyaKR
Copy link
Contributor

PriyaKR commented Nov 6, 2019 •
edited
Loading

Looks good to me.When the media player component is bumped in simorgh this has to be verified on an article page and cross device tested.Cypress tests has to be added then.

Looks good across different browsers.

One thing to mention i can't see the Guidance svg logo in designs but there are in storybook.Are the designs not up to date?

@rhenshaw56
Copy link
Contributor Author

One thing to mention i can't see the Guidance svg logo in designs but there are in storybook.Are the designs not up to date?

@PriyaKR those were modified yesterday, will be addressed in #2562

@PriyaKR
Copy link
Contributor

PriyaKR commented Nov 6, 2019

ok thanks @rhenshaw56 this can be merged also cypress tests can be added now have created a separate issue bbc/simorgh#4580 and blocked it until we update the psammead media player to version 2.2.0 in simorgh

@rhenshaw56 rhenshaw56 merged commit 79c20ef into latest Nov 6, 2019
@PriyaKR PriyaKR deleted the 3139_guidance branch November 6, 2019 15:52
@RayNjeri RayNjeri mentioned this pull request Nov 6, 2019
Merged
4 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@simonsinclair simonsinclair simonsinclair approved these changes

@HarveyPeachey HarveyPeachey HarveyPeachey approved these changes

@12 12 12 approved these changes

@dr3 dr3 dr3 approved these changes

@jamesbhobbs jamesbhobbs Awaiting requested review from jamesbhobbs

@j-pendlebury j-pendlebury Awaiting requested review from j-pendlebury

@jroebu14 jroebu14 Awaiting requested review from jroebu14

@sareh sareh Awaiting requested review from sareh

@jamesdonoh jamesdonoh Awaiting requested review from jamesdonoh

Assignees

@PriyaKR PriyaKR

@rhenshaw56 rhenshaw56

Labels
a11y-swarm An a11y swarm (clarify dev or full team in the desc) needs to be carried out before moving to test articles-av-epic ux To be reviewed by UX before merging ws-articles Tasks for the WS Articles Team ws-media The World Service media stream
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AV: Add guidance to media placeholder image
7 participants
@rhenshaw56 @jamesdonoh @PriyaKR @simonsinclair @HarveyPeachey @12 @dr3