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

Add title attribute to media player #2434

Merged
merged 12 commits into from
Oct 18, 2019
Merged

Add title attribute to media player #2434

merged 12 commits into from
Oct 18, 2019

Conversation

pharingee
Copy link
Contributor

@pharingee pharingee commented Oct 18, 2019 •
edited
Loading

Resolves #2403

Overall change:
Add a title prop which is passed down to both amp and canonical iframes

Code changes:

  • Add title attribute to players
  • Update tests
  • Update stories
  • 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

@pharingee pharingee self-assigned this Oct 18, 2019
@pharingee pharingee added ws-media The World Service media stream a11y Accessibility-related task ws-media-asset-page-v1 labels Oct 18, 2019
pjlee11
pjlee11 suggested changes Oct 18, 2019
View reviewed changes
Copy link
Contributor

@pjlee11 pjlee11 left a comment

Choose a reason for hiding this comment

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

Couple of small tweaks needed

packages/components/psammead-media-player/CHANGELOG.md Outdated Show resolved Hide resolved
packages/components/psammead-media-player/package.json Outdated Show resolved Hide resolved
@pharingee pharingee requested a review from pjlee11 October 18, 2019 10:11
@pjlee11
Copy link
Contributor

pjlee11 commented Oct 18, 2019 •
edited
Loading

Just removing -alphaX from the version number doesn't stop npm publishing it as a alpha, you need to remove the following from the package.json and then install to update the lock

  "publishConfig": {
    "tag": "alpha"
  },

@pjlee11
Copy link
Contributor

pjlee11 commented Oct 18, 2019

Could you update the READMEto include the title prop for both AMP and canonical. Also mention in Accessibility notes that the title is used for a11y best practice on the iframe.

pjlee11
pjlee11 approved these changes Oct 18, 2019
View reviewed changes
Copy link
Contributor

@pjlee11 pjlee11 left a comment

Choose a reason for hiding this comment

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

Nice work 😎

ibMadbouly
ibMadbouly approved these changes Oct 18, 2019
View reviewed changes
Copy link
Contributor

@ibMadbouly ibMadbouly left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@EinsteinNjoroge
Copy link
Contributor

You might wanna check some of these boxes in the description.
Screen Shot 2019-10-18 at 4 49 40 PM

@pharingee pharingee merged commit 8d2d5db into latest Oct 18, 2019
@amywalkerdev amywalkerdev deleted the ifram-title-props branch October 23, 2019 17:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pjlee11 pjlee11 pjlee11 approved these changes

@ryanmccombe ryanmccombe ryanmccombe approved these changes

@dr3 dr3 dr3 approved these changes

@chris-hinds chris-hinds chris-hinds approved these changes

@EinsteinNjoroge EinsteinNjoroge EinsteinNjoroge approved these changes

@ibMadbouly ibMadbouly ibMadbouly 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

Assignees

@pharingee pharingee

Labels
a11y Accessibility-related task ws-media The World Service media stream ws-media-asset-page-v1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add title attribute to the Media Player component
7 participants
@pharingee @pjlee11 @EinsteinNjoroge @ryanmccombe @dr3 @chris-hinds @ibMadbouly