-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Added disableAlpha prop to CustomGradientPicker and GradientPicker components #66974
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @wwdes. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @wwdes! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
f1c6608
to
1fdacb4
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.
Thanks for working on it @wwdes!
Generally looks good, I've left a few minor suggestions.
The only thing I'm worried about is the prop name. ColorPicker
's disableAlpha
prop is deprecated in favor of the opposite enableAlpha
prop. Ideally it would be nice to follow the same pattern for all components that use ColorPicker
under the hood. Whether we want to do it here or in another PR, I'd defer to @ciampo @mirka who I assume will have more context about our intentions back then.
@@ -213,6 +213,7 @@ export function GradientPicker( { | |||
onChange, | |||
value, | |||
clearable = true, | |||
disableAlpha = false, |
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 will make sense to add the new prop to the GradientPicker
README.
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'll try to get #67250 merged first so we don't need to do this manually 🙂
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.
Done. @wwdes If you rebase on or merge back trunk
, running npm run docs:components
should automatically update the README according to the TypeScript data.
/** | ||
* Whether to disable alpha transparency options in the picker. | ||
*/ | ||
disableAlpha?: boolean; |
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.
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.
The only thing I'm worried about is the prop name.
ColorPicker
'sdisableAlpha
prop is deprecated in favor of the oppositeenableAlpha
prop. Ideally it would be nice to follow the same pattern for all components that useColorPicker
under the hood. Whether we want to do it here or in another PR
Yes, I think we should flip it to be enableAlpha
, if only for consistency between relevant components. And we should do it in this 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.
If we flip the prop to enableAlpha, what should the default value be? ColorPicker defaults it to false, but GradientPicker was always set to have alpha enabled.
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.
We don't want to change the default behavior, so the default should be true
👍
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.
In that case, I'll make the flip within GradientPicker component, and leave disableAlpha prop in custom-gradient-picker and custom-gradient-bar as is.
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.
OK, the changes are made.
I changed the prop name to enableAlpha in both GradientPicker and CustomGradientPicker components and the flip is done within CustomGradientPicker. I left the disableAlpha prop as it was in CustomGradientBar.
Note that I rolled back previous commits and did everything in one go instead while also updating and rebasing trunk. However, I am not sure that I did everything correctly regarding git workflow.
8835923
to
acf91da
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.
Looks good and tests well! I also confirmed that nothing problematic happens in GradientPicker when a user chooses a gradient preset with an alpha (e.g. linear-gradient(135deg,rgba(6,147,227,0.5) 0%,rgb(155,81,224) 100%)
) while enableAlpha
is false. Nothing blows up for the existing alpha gradient, just the alpha slider is disabled.
I just had some small formatting nits. Everything else is ready to go!
Co-authored-by: Lena Morita <[email protected]>
Co-authored-by: Lena Morita <[email protected]>
Co-authored-by: Lena Morita <[email protected]>
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.
Great work here, thank you!
Congratulations on your first merged pull request, @wwdes! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts: https://profiles.wordpress.org/me/profile/edit/ And if you don't have a WordPress.org account, you can create one on this page: https://login.wordpress.org/register Kudos! |
What?
This PR adds disableAlpha prop to GradientPicker component, which is already supported within CustomGradientBar subcomponent.
Why?
Firstly, both ColorPicker and ColorPalette component's have this feature under enableAlppha prop, so adding it to GradientPicker would only be consistent. Secondly, I have found a use for this feature within my own project, where a block can not be allowed transparency, but wants to utilize other features of the GradientPicker component.
How?
The GradientPicker's subcomponent's subcomponent "CustomGradientBar" already supports disabling alpha for custom gradients. All i did was add disableAlpha prop to GradientPicker and it's subcomponent, CustomGradientPicker, in order to take advantage of the feature within CustomGradientBar component.
This is my first attempt at contributing, and I do not expect it to be free of faults.
Testing Instructions
When using GradientPicker component, simply add the disableAlpha prop with 'true' as the value, or omit the prop to let it resort to default value of 'false'.