-
Notifications
You must be signed in to change notification settings - Fork 177
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
Code: Improved codebase by minimizing package interdependency #12344
Conversation
// Temporary solution for animations. Better would be to move this | ||
// prop type to the types package, but really? | ||
animations: PropTypes.arrayOf(PropTypes.object), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted, this isn't great, but okay until we convert this package to TypeScript.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better would be to move this prop type to the types package
Is this referring to just the animation prop type? Or all of StoryPropTypes
in general?
Technically it would make sense for all of the StoryPropTypes.*
prop types to live next to their respective TS counterparts, e.g. StoryPropTypes.page
in the story
package, StoryElementPropTypes
in elements
, and so on.
But yeah I guess with the conversion to TypeScript the prop types usage will slowly fade away anyway (unless we see a lot of value in keeping it around)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just this single prop type, because the dependency between the animation and the elements package have been reversed. Yes, it would make sense to move them all to the types package, I'll look at that in #12126.
@@ -42,17 +42,17 @@ import { getMediaSizePositionProps } from '@googleforcreators/media'; | |||
|
|||
const PRECISION = 1; | |||
|
|||
export function getMediaBoundOffsets({ element }) { | |||
export function getElementOffsets(element) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I know that this function only makes sense for media elements (that have scale and focal values), but I've made it general anyway (just returning all zeros for non-media).
const hasOffset = isMedia | ||
? getHasElementOffsets(selectedElement) | ||
: DEFAULT_HAS_OFFSETS; | ||
const normalizedScale = progress(selectedElement.scale || 0, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really sure if the isMedia
guard is even needed here. The function will work on background shapes just fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels like leaving it helps better understand the intent here
Size Change: -59 B (0%) Total Size: 2.71 MB ℹ️ View Unchanged
|
Plugin builds for f42a43d are ready 🛎️!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG, I saw no regressions.
Thank you for the detailed areas that need testing @barklund . Video strip generation Test web-story: https://stories-qa-wordpress-amp.pantheonsite.io/wp-admin/post.php?post=19878&action=edit#page=%25225f845a7e-3e7b-4d18-8d75-2d6fb062f67a%2522 Confirmed that for videos that have enough frame information, it is possible to hit the :scissors icon and trim the video. It is also possible to re-trim back from the original source on subsequent attempts. Test case permutations:
Negative test cases:
|
Paste text Confirmed that copying text from another source and pasting into the canvas creates a text element that has default font/styling : left justified, Roboto font, Regulat Size 18 with 1.2 line spacing black & white. Test Case Variations:
|
Media scale Confirmed scaling for images and video elements from 100% to 400% Test Web Story: https://stories-qa-wordpress-amp.pantheonsite.io/wp-admin/post.php?post=19942&action=edit#page=%2522ee551669-8fb8-4983-8fc6-a5d6836ba7f7%2522 Test Case Variations:
|
Media Pan Animation Confirmed that without scaling that the image is too small to allow PAN up or down. But side PAN left and right are supported. Confirmed that after scaling up from 100%, immediately the PAN up and down are available depending on how the scaling was done. Test Case Permutations:
|
@barklund -- here is the set of inconsistencies that I wanted to bring to your attention: Video strip generation Paste text Media Pan Animation
Issue: The animation becomes None and there is a console error Expected: I was expecting the PAN left to become PAN right when I clicked the right arrow. Instead I get a console error and the animation is removed.
|
Thanks @kkalarickal for the thorough report. All of the 4 highlighted issues also occur on main, so these are not regressions (though still bugs). I will file them independently as such and merge this PR. |
Context
This moves some code and/or responsibility from one package to another in four areas:
getDefinitionForType
instead, which would instead add a link to the elements package ingetMediaBounds
function)generateVideoStrip
to the story-editor package)BG_SCALE_*
properties toMediaEdit
from the story editor)(content: string) => void
tousePasteTextContent
)Testing Instructions
To make sure no regressions are introduced, please thoroughly test these bits of functionality:
Video strip
Media pan animation
Media scale
Paste text
Checklist
Type: XYZ
label to the PRFixes #12332