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

CI-470 Extract width from video.scss #131

Merged
merged 3 commits into from
Feb 18, 2021

Conversation

AliceArmstrong
Copy link
Contributor

@AliceArmstrong AliceArmstrong commented Feb 2, 2021

Ticket.

YouTube videos are to be used with different widths in different places, so this PR extracts the width from the component so that we can set it where we use the component, giving greater flexibility.

To test:

Needs to go live at the same time as this PR.

@AliceArmstrong AliceArmstrong changed the title Extract width from video.scss CI-470 Extract width from video.scss Feb 2, 2021
@NickColley
Copy link
Contributor

Do we need to consider how the other consumers of this package might be impacted?

@AliceArmstrong
Copy link
Contributor Author

AliceArmstrong commented Feb 8, 2021

Do we need to consider how the other consumers of this package might be impacted?

We do, I've mentioned some repos it might affect on the description in the next-article PR. I've listed the repos that mention the CSS class in their tests, which leads me to believe that they may use YouTube videos in the same way, but I'm not really sure how to verify it.

@NickColley
Copy link
Contributor

NickColley commented Feb 8, 2021

I suppose if we're confident we could add the instructions of what to check and update to the release notes as this'll be a breaking change, then this'd delegate that decision making to people using this dependency.

Might be worth checking with Josh what we should be recommending people update the presentation to be.

@GlynnPhillips
Copy link
Contributor

I think this is fine, the width of a video is granular style that probably shouldn't be decided by a global component used by multiple consumers.

If we release this as a breaking change and document what breaks in the release notes then that should be ok. We can then just update the apps we care about to the latest version

@AliceArmstrong
Copy link
Contributor Author

@GlynnPhillips @NickColley I've updated the migration guide in the readme, let me know if the wording sounds correct 🙏

@kavanagh kavanagh removed their request for review February 15, 2021 19:12
README.md Outdated
@@ -26,6 +26,17 @@ plus common / utility classes used across article elements

## Migration guides

### v9 to v10

v10 moves the responsibility of setting the width of embedded YouTube videos to the consumer, in relation to the 'n-content-video--youtube' class.
Copy link
Contributor

@NickColley NickColley Feb 18, 2021

Choose a reason for hiding this comment

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

Now that we've confirmed that the decisions decision is to have full width videos across FT.com we could consider re-wording this so people know they don't need to make a change.

We can keep the steps below if they do need to bring back the previous behaviour for some reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, looking into it now.

@AliceArmstrong AliceArmstrong requested a review from a team as a code owner February 18, 2021 10:47
Copy link
Contributor

@NickColley NickColley left a comment

Choose a reason for hiding this comment

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

Nice, thanks. 👍🏻

@AliceArmstrong AliceArmstrong merged commit ff16fc7 into master Feb 18, 2021
@AliceArmstrong AliceArmstrong deleted the live-blogs-display-youtube-embeds-full-width branch February 18, 2021 10:52
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.

None yet

3 participants