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

Correctly sets YouTube atom data attributes #256

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

SHession
Copy link

@SHession SHession commented Jun 7, 2021

What does this change?

This PR aims to fix a user reported issue with the Teleporter Copy Embed Link behaviour for YouTube atoms. The way the current data attributes are configured means the outputted link is incorrect. The data-atom-id should be set to the id of the media atom and the data-atom-type should be set to "media".

This change is designed to account for the issues a change to the interface might cause by making the new property optional. The upgrade is also only significant for DCR, as this is what Teleporter uses.

How to test

I have attempted to test this using npm link but I found the video player didn't work with or without my change. It does however work when the package isn't linked. I would be interested in pairing on testing this if anyone knows where I'm going wrong.

How can we measure success?

Users can press the Copy Embed Link button in Teleporter and be confident it will work as expected.

Have we considered potential risks?

Any change to the interface can be frustrating to implement in each of the consumers. This change attempts to mitigate problems by making the necessary parameter optional.

Accessibility

@github-actions
Copy link
Contributor

github-actions bot commented Jun 7, 2021

Size Change: +36 B (0%)

Total Size: 54 kB

Filename Size Change
dist/commonjs/YoutubeAtom.js 987 B +10 B (+1%)
dist/commonjs/YoutubeAtomPlayer.js 2.03 kB +9 B (0%)
dist/esm/YoutubeAtom.js 911 B +9 B (+1%)
dist/esm/YoutubeAtomPlayer.js 1.89 kB +8 B (0%)
ℹ️ View Unchanged
Filename Size
dist/commonjs/Answers.js 1.26 kB
dist/commonjs/AudioAtom.js 2.74 kB
dist/commonjs/ChartAtom.js 400 B
dist/commonjs/common/MaintainAspectRatio.js 422 B
dist/commonjs/expandableAtom/Body.js 979 B
dist/commonjs/expandableAtom/Container.js 769 B
dist/commonjs/expandableAtom/Footer.js 1.17 kB
dist/commonjs/expandableAtom/Summary.js 1 kB
dist/commonjs/ExplainerAtom.js 664 B
dist/commonjs/GuideAtom.js 596 B
dist/commonjs/index.js 504 B
dist/commonjs/InteractiveAtom.js 494 B
dist/commonjs/InteractiveLayoutAtom.js 443 B
dist/commonjs/KnowledgeQuiz.js 2.42 kB
dist/commonjs/lib/formatTime.js 384 B
dist/commonjs/lib/ophan.js 320 B
dist/commonjs/lib/pillarPalette.js 273 B
dist/commonjs/lib/unifyPageContent.js 694 B
dist/commonjs/PersonalityQuiz.js 2.6 kB
dist/commonjs/Picture.js 1.65 kB
dist/commonjs/ProfileAtom.js 592 B
dist/commonjs/QandaAtom.js 574 B
dist/commonjs/SharingIcons.js 881 B
dist/commonjs/TimelineAtom.js 1.21 kB
dist/commonjs/types.js 97 B
dist/commonjs/VideoAtom.js 522 B
dist/commonjs/YoutubeAtomOverlay.js 1.15 kB
dist/commonjs/YoutubeAtomPlaceholder.js 363 B
dist/esm/Answers.js 1.14 kB
dist/esm/AudioAtom.js 2.68 kB
dist/esm/ChartAtom.js 330 B
dist/esm/common/MaintainAspectRatio.js 361 B
dist/esm/expandableAtom/Body.js 907 B
dist/esm/expandableAtom/Container.js 706 B
dist/esm/expandableAtom/Footer.js 1.1 kB
dist/esm/expandableAtom/Summary.js 944 B
dist/esm/ExplainerAtom.js 606 B
dist/esm/GuideAtom.js 530 B
dist/esm/index.js 244 B
dist/esm/InteractiveAtom.js 433 B
dist/esm/InteractiveLayoutAtom.js 387 B
dist/esm/KnowledgeQuiz.js 2.33 kB
dist/esm/lib/formatTime.js 317 B
dist/esm/lib/ophan.js 252 B
dist/esm/lib/pillarPalette.js 224 B
dist/esm/lib/unifyPageContent.js 635 B
dist/esm/PersonalityQuiz.js 2.48 kB
dist/esm/Picture.js 1.58 kB
dist/esm/ProfileAtom.js 528 B
dist/esm/QandaAtom.js 507 B
dist/esm/SharingIcons.js 814 B
dist/esm/TimelineAtom.js 1.14 kB
dist/esm/types.js 31 B
dist/esm/VideoAtom.js 467 B
dist/esm/YoutubeAtomOverlay.js 1.07 kB
dist/esm/YoutubeAtomPlaceholder.js 305 B

compressed-size-action

@SHession SHession marked this pull request as draft June 7, 2021 10:24
@SHession SHession marked this pull request as ready for review June 7, 2021 10:38
@SHession SHession force-pushed the fix-youtube-embed-link branch from f06bb55 to c83e67f Compare March 8, 2022 16:09
@SHession SHession force-pushed the fix-youtube-embed-link branch from c83e67f to ea6ebc1 Compare March 8, 2022 16:12
@joecowton1
Copy link
Contributor

@SHession do you know if this is still required?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants