Reworking the elements package
#12557
Conversation
# Conflicts: # package-lock.json # packages/elements/src/elementType.js # packages/elements/src/elementType.ts # packages/elements/src/types.js # packages/elements/src/types.ts # packages/elements/src/types/element.ts # packages/elements/src/types/page.ts # packages/elements/src/types/propTypes.ts # packages/elements/src/utils/createNewElement.ts # packages/elements/src/utils/createPage.ts # packages/elements/src/utils/duplicateElement.ts # packages/elements/src/utils/duplicatePage.ts # packages/elements/src/utils/getElementOffsets.ts # packages/elements/src/utils/getElementOrigin.ts # packages/elements/src/utils/getLayerName.ts # packages/elements/src/utils/getTransformFlip.ts # packages/elements/src/utils/isElementBelowLimit.ts # packages/elements/tsconfig.json # packages/migration/package.json # packages/templates/src/raw/a-day-in-the-life/index.ts # packages/units/src/context.ts # packages/units/src/dimensions.ts # packages/units/src/getBoundRect.ts # packages/units/src/types.ts # tsconfig.json
barklund
added
Type: Code Quality
|
This PR should be renamed removing the types package. |
Yeah, that's why I wanted to convert both packages to put these in the element library, but that's turned out to be too much work (a lot of the components are tricky), so the element types are just here in the elements package until we get to the element library package. With the element library already depending on the elements package, I didn't think this was a big problem. I we can put them in the element library right away without converting the rest of that package, I'm all for it, but didn't know the right way to do that. |
|
Plugin builds for f1e7468 are ready 🛎️!
|
|
Size Change: +39 B (0%) Total Size: 2.72 MB ℹ️ View Unchanged
|
In Here's an example: https://github.com/GoogleForCreators/web-stories-wp/tree/try/element-library-types This works well, the only problems are Since these packages shouldn't really rely on For For |
Thanks, I'll work through these and see if I can find a good solution! |
We already had the Thanks for the hints! |
| @@ -40,6 +46,8 @@ export interface Mask { | |||
| name?: string; | |||
| path?: string; | |||
| ratio?: number; | |||
| iconPath?: string; | |||
| iconRatio?: number; | |||
| supportsBorder?: boolean; | |||
| } | |||
There was a problem hiding this comment.
Wondering: should the Mask type better live in the masks package?
There was a problem hiding this comment.
The masks package depends on the elements package, and that dependency is pretty locked-in. There are all the components for display, frame, and output, that have element typings as well as several util functions (shouldDisplayBorder, canMaskHaveBorder, ...). And the big one is the BorderedMaskedElement using many element features.
Some of these could be solved by utilizing DimensionableElement from units package (though the border in that package does not include a color which masks need, which would add a dependency on the patterns package, but that's probably okay), others could be solved by also adding a MaskableElement to the masks package, but we still also need things like the flip and isBackground properties from the elements (in the BorderedMaskedElement), so that would require making even more element interfaces.
It's doable, but it's getting a bit complex (a naming is getting terrible 😂). Let's move this idea to a new ticket.
| isMedia?: boolean; | ||
| getLayerText: (element: E) => string; | ||
| defaultAttributes: Partial<E>; | ||
| Edit: React.FC<EditProps<E>>; |
There was a problem hiding this comment.
I've seen in multiple articles / resources mentioned that React.FC is better not to use, e.g. because it causes the component to implicitly accept children. I think there were some other reasons as well. Thoughts? You probably have more information about it.
There was a problem hiding this comment.
That has been changed in React 18. React.FC (or React.FunctionComponent) no longer implicitly accepts children.
BUT we're not using React 18, and probably never will because of all the migration issues. So you're right, it should probably be changed to a more clean type.
There was a problem hiding this comment.
React.VoidFunctionComponent seems to be the way to do it pre-18. Thanks!
There was a problem hiding this comment.
Nice! Should we add the eslint configuration as well to prefer this to React.FC mentioned in the comment you shared? cc @swissspidy
There was a problem hiding this comment.
Sure, why not, perhaps in a new PR?
Also fixed a test. Note that there are many more instance of type:"page" in the code, but most are related to tests that will probably still work fine or templates, which will also work fine with an extra unused property. Should we add a migration though?
|
I think all the remaining errors come from the fact, that @merapi, do you have any idea how that can be? I really don't get it? It's probably something obvious, like a missing property on some element that has to be there, but why doesn't it throw an error then? Why just nothing? |
|
Made that same observation here: |
# Conflicts: # tsconfig.json
…e/12126-ts-elements
|
There's a new issue in the config providers, because they somehow can't depend on the element library, but I actually also don't think they should. We can move the font definitions to the elements package, that seems like a decent place for it. The product type returned by the Where should the new non-element product interface live though? Also in elements? We need it in both the dashboard and and the story-editor packages. Thoughts, @miina, @swissspidy? |
Yeah that sounds wrong, it should return a „ProductData“ type or something.
I don‘t think dashboard does anything with product data (except for testing that the API endpoint works, for which we don‘t really need types). Since ProductData is used by both the editor and the product element type, moving to elements might be easiest for now. In the future it could even be its own package though |
# Conflicts: # packages/media/src/createResource.ts # packages/media/src/types/propTypes.ts # packages/wp-story-editor/src/api/utils/normalizeResourceSizes.js
|
@barklund - I spent some time smoke testing the Basically, in a single browser instance, I went through the following:
The functionality follows whatever the behavior is on master-branch. There were a bunch of console errors that I wanted to share with you:
|
Good to hear.
Hmm, that's weird. I'll check if it means anything. |
I'm pretty sure they're unrelated (comes from other plugins). Let's proceed. |
|
@kkalarickal, merging this to unblock a number of other issues. If you do end up finding something, please alert me and I'll fix it asap! |
* Moved types around and converted elements package * WIP: convert element-library package * Re-moved types to relevant packages and untyped the element-library * Updating package.json * Feedback improvements * Fixed more from PR feedback * Fix lint error * Moved element definitions to element library package and updated dependencies * Fixed React-17 React.FC issue and resource id reference * Removed Page.type property as it is redundant Also fixed a test. Note that there are many more instance of type:"page" in the code, but most are related to tests that will probably still work fine or templates, which will also work fine with an extra unused property. Should we add a migration though? * Fix imports of background audio prop type * Fixed a karma test * Finally fixed that annoying bg-shape-element copy thingy * Added a comment to make sense of it * Fix eyedropper * Target correct element * Updated type references for config providers * Missing types in package.json? * Reorganized data types to elements package Co-authored-by: Marcin Pietruszka <marcin@webskill.pl>
Context
More work on the elements package, as well as deprecating the types package completely.
(this is slightly WIP - the git merge was a bit muddy to get an overview of, so might make additional changes after creating this PR)
User-facing changes
None (I hope!)
Testing Instructions
Checklist
Type: XYZlabel to the PRFixes #12126