[theme] Remove createV4Spacing from adaptV4Theme
#27072
Conversation
| @@ -207,11 +207,11 @@ describe('adaptV4Theme', () => { | |||
| }).toWarnDev(['adaptV4Theme() is deprecated']); | |||
|
|
|||
| expect(transformedTheme.mixins.gutters()).to.deep.equal({ | |||
| paddingLeft: defaultSpacing * 2, | |||
| paddingRight: defaultSpacing * 2, | |||
| paddingLeft: `${defaultSpacing * 2}px`, | |||
There was a problem hiding this comment.
I would say yes, for people who are using adaptV4Theme. This PR should be released together with the updated codemod, I think.
There was a problem hiding this comment.
Once they run the codemod again, it should be fine.
There was a problem hiding this comment.
So adaptV4Theme requires running the codemod now?
There was a problem hiding this comment.
If this change has merged, yes. Codemod is required to run (or manually remove "px" from {theme.spacing(2)}px).
There was a problem hiding this comment.
But then why would I use the helper? This seems a bit odd that adaptV4Theme requires the codemod and is otherwise broken. Either we make it so that I can apply runtime helper and codemod separately or we remove either one. But this two-step process is prone to user-error i.e. our fault.
Or we tell people nothing about adaptV4Theme and let the codemod apply it. Or rather tell people to not use adaptV4Theme manually.
There was a problem hiding this comment.
Or we tell people nothing about adaptV4Theme and let the codemod apply it. Or rather tell people to not use adaptV4Theme manually.
If I get it right, you are correct. This codemod version #27032 will automatically add adaptV4Theme to the codebase and also remove px from ${theme.spacing(...)}px (with one command). But there is problem because the adaptV4Theme replace .spacing() with v4 behaviour, then the css will be like padding: 10.
This PR will remove the v4 behaviour so projects that haven't migrated can run codemod once and it works.
The only breaking will be the people who already migrate to v5 AND is using adaptV4Theme (which I believe that it is a few). For this group, we can add info to next release note that they should run codemod once more.
Is this clear to you?
There was a problem hiding this comment.
There is another option, which does not need this PR.
The codemod does not fix the theme.spacing and let adaptV4Theme do it. However, once they complete migration and they want to remove adaptV4Theme, they will need to remove px from theme.spacing manually or run a separate codemod.
In my opinion, I favor the one-time codemod.
There was a problem hiding this comment.
@eps1lon is this sounds good to you?
oliviertassinari
changed the title
createV4Spacing from adaptV4ThemecreateV4Spacing from adaptV4Theme
createSpacingV4conflicts with theme-spacing codemodAs a result, you currently cannot use codemod together with adaptV4Theme.