Skip to content
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

Update the embed block to be a custom video embed that uses the design system video embed template #62

Merged
merged 17 commits into from
Jan 15, 2025

Conversation

helenb
Copy link
Contributor

@helenb helenb commented Jan 3, 2025

What is the context of this PR?

See https://jira.ons.gov.uk/browse/CMS-204

How to review

Screenshots

Click to expand

With cookies accepted

Screenshot 2025-01-03 at 09 20 56

Without cookies accepted

Screenshot 2025-01-03 at 09 15 15

Follow-up Actions

@helenb helenb requested a review from a team as a code owner January 3, 2025 08:57
@helenb helenb marked this pull request as draft January 3, 2025 08:57
@helenb helenb force-pushed the feature/responsive-embed-CMS-204 branch from ec7e833 to f325aa1 Compare January 3, 2025 09:24
@helenb helenb marked this pull request as ready for review January 3, 2025 09:28
Copy link
Contributor

@zerolab zerolab left a comment

Choose a reason for hiding this comment

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

Added a couple of quick thoughts

cms/core/blocks/embeddable.py Outdated Show resolved Hide resolved
cms/core/blocks/embeddable.py Show resolved Hide resolved
@zerolab zerolab requested a review from RealOrangeOne January 7, 2025 11:57
@helenb helenb force-pushed the feature/responsive-embed-CMS-204 branch from b162bf1 to d45b7ba Compare January 7, 2025 13:12
Copy link
Contributor

@nehakerung nehakerung left a comment

Choose a reason for hiding this comment

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

LGTM - just did a functional tests and all works as mentioned 🎉

Adding a preview image might be tedious, but since it's DS compliant, I'll ignore it.🫡

@helenb helenb force-pushed the feature/responsive-embed-CMS-204 branch from d45b7ba to 70887da Compare January 7, 2025 14:07
@helenb helenb force-pushed the feature/responsive-embed-CMS-204 branch from f8a7305 to c4dc506 Compare January 7, 2025 14:24
@helenb
Copy link
Contributor Author

helenb commented Jan 7, 2025

LGTM - just did a functional tests and all works as mentioned 🎉

Adding a preview image might be tedious, but since it's DS compliant, I'll ignore it.🫡

Thanks @nehakerung!

cms/core/blocks/embeddable.py Outdated Show resolved Hide resolved
@zerolab zerolab requested review from AdamHawtin and ababic January 10, 2025 10:21
cms/core/tests/test_video_embed_block.py Outdated Show resolved Hide resolved
cms/core/blocks/embeddable.py Outdated Show resolved Hide resolved
cms/core/tests/test_video_embed_block.py Outdated Show resolved Hide resolved
cms/core/tests/test_video_embed_block.py Outdated Show resolved Hide resolved
cms/core/blocks/embeddable.py Outdated Show resolved Hide resolved
@nehakerung nehakerung self-requested a review January 14, 2025 14:38
Copy link
Contributor

@nehakerung nehakerung left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@AdamHawtin AdamHawtin left a comment

Choose a reason for hiding this comment

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

Looks good to me, just a suggestion for the unit tests.

cms/core/tests/test_video_embed_block.py Show resolved Hide resolved
Copy link
Contributor

@sanjeevz3009 sanjeevz3009 left a comment

Choose a reason for hiding this comment

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

LGTM =) Tested everything and exhausted all the blocks on the info page and everything is working as it should.

@helenb helenb merged commit 84bf319 into main Jan 15, 2025
9 checks passed
@helenb helenb deleted the feature/responsive-embed-CMS-204 branch January 15, 2025 14:43
ababic pushed a commit that referenced this pull request Jan 16, 2025
* main:
  Update the embed block to be a custom video embed that uses the design system video embed template (#62)
  Django Migration Linter Integration (#43)
  Tidy up the info/warning panel and fix title/no title output (#73)
  Introduce Functional Tests using Playwright and Behave #31

# Conflicts:
#	poetry.lock
#	pyproject.toml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants