-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Remove: __experimentalHasMultipleOrigins prop from colors and gradients #46315
Remove: __experimentalHasMultipleOrigins prop from colors and gradients #46315
Conversation
Size Change: -291 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
I came across this PR while researching #46148 regarding the The problem is that when Should this be addressed in the follow-up PR? |
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.
Looks like there's some related e2e test failures.
@@ -140,10 +164,7 @@ const PanelColorGradientSettings = ( props ) => { | |||
) { | |||
return <PanelColorGradientSettingsInner { ...props } />; | |||
} | |||
if ( props.__experimentalHasMultipleOrigins ) { |
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.
I don't understand the changes on this component, can you detail a bit more?
For me you had two options here:
- Either you could have forced all the controls to be "multiple" or "single" (you choose here the shape of the colors/gradients prop to provide to the lower level components from the components package)
- Or keep the
__experimentalHasMultipleOrigins
prop at this level (block-editor package components) and construct the shape of the palettes depending on this prop, leaving the choice to the consumer of these components.
Added the 'needs dev note' label, so it's flagged to get a mention it in the 6.2 Fieldguide. |
b5568fc
to
e6a2fd9
Compare
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.
This LGTM, you might want to rebase the PR per the recent jobs changes.
efb6e17
to
8468806
Compare
307f273
to
e4daa4e
Compare
…ient related components.
e4daa4e
to
a5fe35e
Compare
Hi @bph, we can write a dev note to get more awareness and testing. But with this PR, users of the string __experimentalHasMultipleOrigins will not notice any change. Our code just got smarter and detects when __experimentalHasMultipleOrigins behavior should be used or not without developers needing to pass it. |
Sorry, catching up through a long queue 😅 Great cleanup work @jorgefilipecosta ! I opened a follow-up PR (#47384) with a few more cleanup tweaks, but nicely done here 🙌 It would be great to see the same treatment applied to the |
Part of #42994.
This PR removes the __experimentalHasMultipleOrigins from all the color and gradient-related components.
Instead of having this property, we now inspect the contents of colors and gradients to understand its shape.
cc: @ciampo
✍️ Dev Note
The
__experimentalHasMultipleOrigins
prop has been removed from theColorPalette
,GradientPicker
,BorderControl
andBorderBoxControl
components in the@wordpress/components
package.The prop has been replaced by a check in the
ColorPalette
component which automatically detects whether the colors passed through thecolors
prop has a single or multiple color origins.