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

DAM-7996 #1747

Merged
merged 12 commits into from
Nov 9, 2023
Merged

DAM-7996 #1747

merged 12 commits into from
Nov 9, 2023

Conversation

raul-wp
Copy link
Contributor

@raul-wp raul-wp commented Oct 17, 2023

Description

This PR is part of the effort of DAM to give an improved experience in vertical videos across Arc products. it's summarized with this changes:

article body

  • Added new custom fields: viewportPercentage, borderRadius & aspectRatio.
  • The aspect ratio of the video is now resolved on the video component or from the user input through the aspectRatio field.

leadart

  • Added new custom fields: viewportPercentage, borderRadius & aspectRatio.
  • The aspect ratio of the video is now resolved on the video component or from the user input through the aspectRatio field.

video-player

  • Added new custom fields: viewportPercentage, borderRadius & aspectRatio.

About the new fields

This three blocks have the same fields and shares the same behavior. Here is a description of each one and what should expect when you interact with them.

Player Aspect Ratio - Handles the aspect-ratio to render for the video player. When you select a value, the block refreshes with the video player rendering at the selected aspect ratio. "Video source" and "--" values renders the player at the aspect ratio of the video. For videos in 9:16 the black background is replaced by a transparent one and the captions gets aligned at the middle of the block.

View height percentage - Handles the height percentage the player takes from viewport. The default value is 65. This field only applies for videos in 9:16; videos in other formats takes the 65%.

Round player corners - Enables the video player to render with rounded corners. This field only applies for videos in 9:16, videos in other formats render with straight edges.

Jira Ticket

Acceptance Criteria

  • Users can use the original aspect ratio of the video or use a specific one in the player.
  • Users can round the corners only for videos in the 9:16 aspect-ratio.
  • Users can modify the height of the video player only for videos in the 9:16 aspect-ratio.

Test Steps

Setup

  1. Go to @wpmedia/arc-themes-components repo
  2. Checkout to the DAM-7996 branch: git checkout DAM-7996
  3. Go back to this repo @wpmedia/arc-themes-blocks
  4. Checkout to the DAM-7996 branch: git checkout DAM-7996
  5. Run fusion repo with linked blocks: npx fusion start -f -l @wpmedia/article-body-block,@wpmedia/lead-art-block,@wpmedia/video-player-block
  6. Create a new Page.
  7. Choose Advanced Right Left as the layout, under the setup menu
  8. Go to the curate menu, And under the fullwidth1 section of the Blocks submenu Insert the Article Body, Lead Art and Video Player blocks
  9. Go to the Global Content section, select content-api as content source and put /test/2023/04/06/story-with-video-featured-media/ in the website_url. Press the update button.
  10. Go to the website selector and choose The Gazette

Article Body

  1. Select the article body block.
  2. Go to the Video Display Options submenu inside the chain configurations section
  3. The new added fields should be displayed
image
  1. Interact with the fields.

Lead Art

  1. Select the Lead Art block.
  2. Go to the Video submenu inside the Custom Fields section
  3. The new added fields should be displayed:
image
  1. Interact with the fields.

Video Player

  1. Select the Video Player block.
  2. Go to the Display settings submenu inside the Custom Fields section
  3. The new added fields should be displayed:
image
  1. Interact with the fields.

Effect Of Changes

Before

Video Player

image No inputs to manage introduced features.

Article Body

image No inputs to manage introduced features.

Lead Art

image No inputs to manage introduced features.

After

Video Player

video-player

Article Body

article-body

Lead Art

lead-art

Dependencies or Side Effects

Author Checklist

  • Confirmed all the test steps a reviewer will follow above are working.
  • Confirmed there are no linter errors. Please run npm run lint to check for errors. Often, npm run lint:fix will fix those errors and warnings.
  • Ran this code locally and checked that there are not any unintended side effects. For example, that a CSS selector is scoped only to a particular block.
  • Confirmed this PR has reasonable code coverage. You can run npm run test:coverage to see your progress.
  • Confirmed this PR has unit test files
  • Ran npm run test, made sure all tests are passing
  • Confirmed relevant documentation has been updated/added.

Reviewer Checklist

The reviewer of the PR should copy-paste this template into the review comments on review.

  • Linting code actions have passed.
  • Ran the code locally based on the test instructions.
    • I don’t think this is needed to be tested locally. For example, a padding style change (storybook?) or a logic change (write a test).
  • I am a member of the engine theme team so that I can approve and merge this. If you're not on the team, you won't have access to approve and merge this pr.
  • Looked to see that the new or changed code has code coverage, specifically. We want the global code coverage to keep on going up with targeted testing.

@raul-wp raul-wp added the ready for review The PR author has completed the PR template and is ready for a review label Oct 19, 2023
@raul-wp raul-wp marked this pull request as ready for review October 19, 2023 19:01
@raul-wp raul-wp requested a review from a team as a code owner October 19, 2023 19:01
vgalatro
vgalatro previously approved these changes Oct 20, 2023
Copy link
Contributor

@vgalatro vgalatro left a comment

Choose a reason for hiding this comment

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

Same here, this looks great, but want one other team member to look it over before merging it.

@vgalatro vgalatro added additional review The PR author has requested multiple reviewers. Do not merge until at least 2 approvals are complete and removed ready for review The PR author has completed the PR template and is ready for a review labels Oct 30, 2023
@blakeganderson
Copy link
Contributor

@raul-wizeline I've been reviewing this PR and the components PR. The components PR looks correct in storybook, and I've confirmed that the changes have gone through to the powa embedMarkup, but I'm not seeing the border radius actually being applied to the videos when I link the blocks. This is what the embed Markup looks like for me, but the video still has sharp corners. Do you know what could be causing this?
image
image

@blakeganderson blakeganderson added the question Further information is requested for the PR author label Nov 3, 2023
@raul-wp
Copy link
Contributor Author

raul-wp commented Nov 8, 2023

@raul-wizeline I've been reviewing this PR and the components PR. The components PR looks correct in storybook, and I've confirmed that the changes have gone through to the powa embedMarkup, but I'm not seeing the border radius actually being applied to the videos when I link the blocks. This is what the embed Markup looks like for me, but the video still has sharp corners. Do you know what could be causing this? image image

Hi @blakeganderson, sorry for the late response;
I forgot to update the powa bundle for themesinternal with the rounded corners feature, It should work now. Apologies for the confusion.

@blakeganderson
Copy link
Contributor

@raul-wizeline the border-radius is working for me now! Feel free to merge

@vgalatro
Copy link
Contributor

vgalatro commented Nov 9, 2023

@raul-wizeline , update the base branch to arc-themes-release-version-2.1.1 before merging, thank you!

@raul-wp raul-wp changed the base branch from arc-themes-release-version-2.1.0 to arc-themes-release-version-2.1.1 November 9, 2023 17:27
@raul-wp raul-wp requested a review from vgalatro November 9, 2023 17:29
@raul-wp raul-wp added the ready for design qa Development complete and approved, ready for design to review label Nov 9, 2023
@nschubach nschubach merged commit 0d1abf8 into arc-themes-release-version-2.1.1 Nov 9, 2023
4 of 6 checks passed
@nschubach nschubach deleted the DAM-7996 branch November 9, 2023 19:40
@pardistaherzadeh
Copy link

pardistaherzadeh commented Nov 9, 2023

Looks good to me! The only comment that I have is to change the label of aspect ratio to 'Player aspect ratio' instead of 'Player Aspect Ratio'. @raul-wizeline

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
additional review The PR author has requested multiple reviewers. Do not merge until at least 2 approvals are complete question Further information is requested for the PR author ready for design qa Development complete and approved, ready for design to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants