Component System: Upgrade FontSizePicker #27594
Conversation
|
Size Change: +122 kB (+10%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
|
Since we're doing this, would it be possible to account for other units as well and not limit this to just pixels? ❤️ |
Indeed! The underlying
|
packages/components/src/__next/context/component-system-provider.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Hi @ItsJonQ nice work on this PR.
It seems we have a good approach to enhance our components 👍
I think it would be nice to move the font size picker to the Gutenberg repository, e.g: if someone needs to debug the code to fix a bug the code of the component is not in this repository.
The component would then use other g2 components inside.
The prototype of g2 includes support for other units and even complex css calculations would it be possible to include the support for this or you prefer to do that in a follow-up PR?
| /** | ||
| * External dependencies | ||
| */ | ||
| import { FontSizeControl } from '@wp-g2/components'; |
There was a problem hiding this comment.
Could we bring the code of FontSizeControl to Gutenberg? Keeping the components used inside FontSizeControl on g2 for now?
There was a problem hiding this comment.
Could we bring the code of FontSizeControl to Gutenberg
To do that, we would need (at minimum) all of the Components (and systems) that make up that control.
I've included a screenshot below of a high-level overview. Required components are in the yellow squares:
Link to Miro board (source of screenshot)
https://miro.com/app/board/o9J_khVkF9A=/?moveToWidget=3074457351837142359&cot=14
Adding the systems and components to build up to FontSizeControl was the original proposal.
However, @gziolo suggested we could try consuming the @wp-g2/components package directly to expedite integration, which I think is a good idea (assuming important things like bundling aren't severely impacted)
There was a problem hiding this comment.
Can't we add FontSizeControl component (just this component) and use its dependencies from the package? The idea is that this will allow us to review the component properly in isolation and step by step we'd do the same for all its dependencies.
| import { __unstableWithNext as withNext } from '../__next/context'; | ||
|
|
||
| export function withNextComponent( current ) { | ||
| return withNext( current, FontSizeControl, 'WPComponentsFontSizePicker' ); |
There was a problem hiding this comment.
I wonder if for something like a FontSizePicker we really need to keep two versions. I guess in this case would be ok if we just deleted the current component and used the new one?
There was a problem hiding this comment.
That's up to us really. It looks like FontSizePicker is only really used in one spot, that being the hooks typography tools.
The thing that is uncertain is if it's used by 3rd party blocks. In that case, I feel like it may be safer to...
- Start with having both
- Ensure it's stable
- Create some documentation for 3rd party devs regarding this migration (context/adapter). Announce this.
- See if there's any pushback
- If there are many people who want to keep the old
FontSizePicker, provide 3rd party devs with the ability to "opt-out" (with maybe deprecation notices) - Wait a bit
- Remove old implementation
☝️ Repeat for components as needed
| const fontSizeValue = | ||
| fontSizeObject?.size || style?.typography?.fontSize || fontSize; | ||
|
|
||
| return <FontSizePicker onChange={ onChange } value={ fontSizeValue } />; |
There was a problem hiding this comment.
Is this mapping necessary here? Wouldn't it be something to put inside the adapter function?
There was a problem hiding this comment.
Is this mapping necessary here?
@gziolo I believe so. style.typography.fontSize is very specific to the hooks/ part of the editor. After testing, it appears that fontSizeValue my change depending on values defined in the theme.json setup. If that's correct, the prop "remap" feels correct in this hooks/ area (vs. in the components package).
|
I already see some good impact with the latest changes applied to
for |
gziolo
force-pushed
the
try/next-font-size-picker-component
branch
from
a33ff2d
to
9b67597
Compare
package-lock.json
Outdated
| "supports-color": "^5.3.0" | ||
| } | ||
| }, | ||
| "commander": { |
There was a problem hiding this comment.
It's surprising to see commander, postcss and rtlcss on the list of dependencies. Is any of them used on the front-end?
|
This error is strange:
It's possible that one of the npm packages bundled with G2 components should be transpiled with Babel. |
|
With 82535b6 we are now at and I only aligned versions for
@ItsJonQ, we should upgrade all libraries that Gutenberg and G2 components share to the latest versions in Gutenberg in a separate PR. Do the same for G2 components. Regenerate the lock file and see how it looks. Maybe we can reduce the size of new code added to a reasonable size and do a more aggressive rollout :) |
gziolo
force-pushed
the
try/next-font-size-picker-component
branch
from
4333452
to
2fea819
Compare
gziolo
added
[Type] Technical Prototype
| @@ -19,7 +19,8 @@ | |||
| "npm": ">=6.9.0" | |||
| }, | |||
| "config": { | |||
| "GUTENBERG_PHASE": 2 | |||
| "GUTENBERG_PHASE": 2, | |||
| "COMPONENT_SYSTEM_PHASE": 0 | |||
There was a problem hiding this comment.
Why is this flag used for? Is't it redundunt with the "context" component provider?
There was a problem hiding this comment.
We want to merge this PR with all the new code removed with Tree Shaking (100kb as of today according to the CI job) when running npm run build. This way we can integrate a few components, remove code duplication wherever possible and enable this flag (or remove completely) afterward.
So this is something that would be only enabled on demand. I think we can enable it by default on Storybook to have the place to preview migrated components.
There was a problem hiding this comment.
What's the point of merging components and not really use them in the plugin?
There was a problem hiding this comment.
I can also confirm now that Tree Shaking works like a charm:
🎉
|
I'm happy to report that Tree Shaking is working perfectly fine 🎉
🎉 Now, I will enable the feature flag again. Locally, I see the following error with the flag enabled:
Let's see if it's confirmed on Travis. I was able to reproduce the same issue without my latest commits. |
|
We are back at:
Some e2e tests are failing so it must be something with the recent changes to the G2 integration. |
I think a dependency (somewhere) isn't providing an ES5 friendly module. Will investigate! |
|
Took a look at the build file. From what I can tell, the
It's really weird... because That one doesn't have 🤔 Not sure how to fix this one. |
|
I think webpack defaults to |
|
@gziolo Interesting!
How do I do that? 🙈 😂 |
gziolo
requested review from
chrisvanpatten,
ellatrix,
nerrad and
ntwb
as code owners
|
@saramarcondes, did we miss any points from your feedback. Are there any blockers? I personally feel we can plan on merging this PR this week behind the feature flag or wait until we create the release branch that is going to be used for WordPress 5.7 beta. In the second case, we don't need the
|
|
Oh boy! All green all around! Thanks for approving @saramarcondes ! @gziolo Looks like we may be ready to land this sometime this week :) |
In that case... I'm gonna do it! 🚀 🤞 |
|
Exciting 🎉✨🔥 |
|
Thanks all. Nice work. We're just getting started |
|
I was looking again at the code merged to idenfity all the G2 components used that we should work on porting next:
@ItsJonQ, what would be your recommendation to tackle next based on the complexity. The lower the better I believe. Should we create a tracking issue to split the work between several people? It's something that we probably could help from other contributors. There are also two imports from
We should also inline two another utils inside
|
|
@gziolo Thank you for the follow up!
I agree! I think we should port these components one at a time. I created a new issue with details on how we can continue with this migration effort: In it, I've listed the components we'll need to migrate in order of dependency and complexity.
Ah yes. We'll need to refactor the component code when we move things order to use the conventions of this codebase.
That seems like something we can do when we move over the components that have unit parsing in them. |
|
Tracking issue looks great, thanks 🙇♂️ |
This update adapts
FontSizePickerfrom@wordpress/componentswith the new Component System setup.It does this by adding the (current) external
@wp-g2component system packages as dependencies and using them directly.This strategy was something @gziolo had suggested here:
#27484 (comment)
How has this been tested?
npm installnpm run devScreenshots
Types of changes
@wp-g2dependenciesCOMPONENT_SYSTEM_PHASEenv flagFontSizePickerto the component system contextChecklist: