-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Cover: Add custom unit support for height controls #20888
Conversation
Size Change: +1.99 kB (0%) Total Size: 863 kB
ℹ️ View Unchanged
|
Hi! I may have missed a detail, but was wondering: what are some intended use cases for allowing the customisation of the unit set beyond px, em, rem, vw, vh? |
lib/client-assets.php
Outdated
* @return array Filtered editor settings. | ||
*/ | ||
function gutenberg_extend_settings_custom_units( $settings ) { | ||
$settings['__experimentalDisableCustomUnits'] = get_theme_support( 'disable-custom-units' ); |
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.
Should this support a second argument where the theme can say which should be the unit shown / used? Like get_theme_support( 'disable-custom-units', 'rem' )
would force rem to be used in all unit-based selectors.
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.
Good suggestion! Let me make that adjustment ✨
@mcsf Haii!! There are many other units beyond the defaults I've set for the base Hope this makes sense 🙏 |
@mtias I just pushed an update! This adds support for additional parameters for For example: // This will disable all custom units
add_theme_support( 'disable-custom-units' );
// This will enable only rem
add_theme_support( 'disable-custom-units', 'rem' );
// This will enable only rem and em
add_theme_support( 'disable-custom-units', 'rem', 'em' ); If, say, only Lemme know how this API feels for you :) In other news... There may be race conditions where a block (by default) expects a We have a bit of this in the case of Cover. To test...
|
This looks good. Yes, there will be several things to tweak to account for units being a "thing" now across blocks, but we'll get there. It might make sense to rename the theme support from |
4eecd7b
to
7feead3
Compare
@mtias Done 👍 |
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've left some notes, but other than that let's keep it moving!
@@ -32,6 +32,9 @@ | |||
"minHeight": { | |||
"type": "number" | |||
}, | |||
"minHeightUnit": { | |||
"type": "string" |
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 assume from this that it is preferred to let components like CoverHeightInput
decide based on an undefined value, correct? Otherwise we might add a default
value here.
style.minHeight = minHeight || undefined; | ||
style.minHeight = minHeightUnit | ||
? `${ minHeight }${ minHeightUnit }` | ||
: minHeight || undefined; |
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 for one had to go check the operator precedence table for logical OR and ternary conditional to make sure I was reading this correctly. :) I recommend parentheses in this situations.
export const BASE = { | ||
black: '#000', | ||
white: '#fff', | ||
}; | ||
|
||
export const G2 = { |
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.
Maybe add TODO-type comment explaining what G2 means (succinctly, focused on why this object is separate from the other colors) and with a note on when it would be a good time to consolidate all of this. Otherwise other devs in a few months won't have a clue of what this means. :)
@mcsf Thank you so much for your feedback 🙏 ! Great suggestions! I just pushed them up |
@mcsf Hmm! It looks like adding the default I'm not sure how to fix it. It looks like the fixture ( 🤔 |
It also means you need a deprecated version for the block, otherwise, it's going to invalidate existing blocks. |
export const COLORS = { | ||
...BASE, | ||
darkGray: DARK_GRAY, | ||
darkGray: merge( DARK_GRAY, G2.darkGray ), |
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 is mutating DARK_GRAY
, such that every reference to DARK_GRAY
would subsequently contain properties of G2.darkGray
. I don't think that's the intention?
darkGray: merge( DARK_GRAY, G2.darkGray ), | |
darkGray: merge( {}, DARK_GRAY, G2.darkGray ), |
A few other instances in this file as well.
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.
Oh! I didn't know it mutates. Thank you! Will fix those up
0a99fa5
to
dd9e79d
Compare
Rebased with latest |
dd9e79d
to
d1469b6
Compare
@youknowriad Haiii! I'm following up on this PR :). I'm not sure how to proceed with deprecation. For this particular change, would adding a new Thank you! |
The block-editor variant of UnitControl has add_theme_support built in
…ble-custom-units' )`
6d1f7d8
to
95a7024
Compare
Yay! Looks like Travis green and happy! Excited for this one to land. Merging it up! |
|
||
const baseUnitLabelStyles = ( props ) => { | ||
return css` | ||
appearance: none; |
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'm testing this on Firefox 75 in a Linux OS and the input control in Firefox includes the stepper arrows buttons:
while in Chrome it doesn't:
For what is worth, I've tried with -moz-appearance: textfield
and, as far as my testing went, it worked as expected (arrow up/down with the keyboard still increases/decreases the number) and the steppers are not shown.
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'm testing this on Firefox 75 in a Linux OS
Firefox (76) on macOS is also affected.
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.
Potential fix at #21571
For people like me who don't know why this option is not available in the editor of cover block and find that PR, it's because the theme didn't enable custom units: add_theme_support( 'custom-units' ); // this allow the post author to select other CSS units than "px" |
Description
This update adds a new (experimental)
UnitControl
that enables values beyond pixels! For Cover block, this control unlocks the ability to setpx
,em
,rem
,vw
andvh
values.(Note: Although
%
is supported, it's omitted from the Cover block control as%
height does not render predictably).How has this been tested?
UnitControl
andNumberControl
were tested in Storybook and GutenbergUnitControl
x Cover block integration was developed and tested in local GutenbergScreenshots
The GIF above showcases the flow for changing values and unit values for the Cover block.
Types of changes
UnitControl (and NumberControl)
These new experimental components have been added to
@wordpress/components
. TheUnitControl
is the component that powers the input for setting the height for the Cover block.Shift Increment
One of the features with this new control is to jump values when holding down
shift
while pressingup
ordown
(default is by10
). This interaction is common for other Design applications like Figma, Sketch, Photoshop, etc...Note: This feature can be customized and even disabled in the base component.
Switching Units
The original behaviour defaults to a min-height of
50px
, but only if the user makes a change. This interaction is preserved in this update.To improve the canvas rendering of Cover, switching units would "reset" the Cover to predetermined values for each unit. This is to prevent the Cover block's height from unexpectedly jumping to
3000px
if they switch to avh
orvw
unit.Drag Resize Handling
Dragging to resize will "reset" the unit back to px. This interaction decision was made as it is very difficult to accurately translate pixel by pixel drag values to their respective
vh
orem
values.Simulated Height Rendering
Cover's canvas rendering is simulated as best as possible given the value and unit type. For example,
vh
andvw
can be accurately rendered. Whereas we have to take the best guess for units likeem
orrem
.Tests
These updates have been tested locally in Gutenberg. Unit tests need to be written for the new
UnitControl
andNumberControl
components.Checklist:
Resolves:
#20567 (comment)