Allow setting viewport tablet and mobile values in theme.json#79104
Allow setting viewport tablet and mobile values in theme.json#79104tellthemachines wants to merge 8 commits into
Conversation
|
Size Change: +1.21 kB (+0.01%) Total Size: 8.6 MB 📦 View Changed
|
|
note: switched to draft as I didn't intend to mark it ready for review just yet 😅 mostly what's still missing is integration with the device preview, but I mean to test it a bit futher too |
|
Flaky tests detected in 4850ae5. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/27740405987
|
82eee69 to
b5beab9
Compare
| * These are available for all blocks and wrap their styles in the given media query. | ||
| * Keep in sync with RESPONSIVE_BREAKPOINTS in packages/global-styles-engine/src/core/render.tsx. | ||
| * | ||
| * @deprecated Use get_responsive_media_queries() instead. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
b5beab9 to
40efcb2
Compare
|
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 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. |
|
This PR is now ready for review! It's working pretty well in my testing. The only oddities I'm coming up against are the legacy responsive controls on certain blocks, which have hardcoded breakpoints that don't always match even the defaults:
it would be nice if these could all use a configurable breakpoint (but which one? and if we ever allow configuring the amount of breakpoints, how will we pick one to assign here? or will we make that configurable too?) But I'm happy to just ignore them for now because these controls will eventually be made obsolete by viewport states anyway 😅 |
ramonjd
left a comment
There was a problem hiding this comment.
I tested with global and block styles with various values in units in settings.viewport, and it's working pretty well out of the box. Values are hooked up to the device preview dropdown 👍🏻
I got into the weeds a bit and didn't test style variations yet
Kudos for getting this started
| * @param array $breakpoints Viewport breakpoint sizes. | ||
| * @return bool Whether the breakpoint order is valid. | ||
| */ | ||
| private static function is_valid_viewport_breakpoint_order( $breakpoints ) { |
There was a problem hiding this comment.
how does this go with mixed unit breakpoints, e.g., { mobile: "70rem", tablet: "960px" }.
is it possible to have overlapping or impossible media ranges?
There was a problem hiding this comment.
Yes! I left a note about that in point 3 of the testing instructions. If units are different and tablet is smaller than mobile, the tablet query is still output but obv doesn't work. This is easier than doing a whole lot of work to translate between units to check their relative sizes, and imo really it should be the theme author's responsibility to put in values that make sense. What do you think?
There was a problem hiding this comment.
Ah apologies, I was in the weeds!
Yeah, agree it should be the author's responsibility.
If the tablet value is smaller than mobile and uses the same unit, the values will be silently replaced with the default ones.
I was gonna say, what if we rejected mixed values, but this sounds like a good compromise. 👍🏻
There was a problem hiding this comment.
yeah I don't think we should reject mixed values if they work correctly!
| }; | ||
|
|
||
| function getPixelValue( value: string ) { | ||
| if ( ! value.endsWith( 'px' ) ) { |
There was a problem hiding this comment.
Not for this PR, but is there a way to handle block previews in global styles for values are rem or something other than px so that they reflect the current state?
There was a problem hiding this comment.
The way responsive previews currently work (both the device preview and the block previews in global styles) is by adding the breakpoint value to the iframe as width. That'll work fine in whichever unit, but the block previews currently have hardcoded values of 480 and 600px for mobile and tablet. We should be able to change that; I'll have a look.
There was a problem hiding this comment.
Ok so I'd forgotten about the scaling that happens further down and expects a unitless px value. That's also what the BlockPreview component expects; unfortunately none of our theme-building UI seems to have taken into account some fairly common theming preferences 😅
Without redoing the logic in BlockPreview, we can get a pretty good approximation of accuracy by converting em and rem assuming a default font size of 16. The fact that the iframe width ends up being set in px will only break things a bit if users have their browser font set to a non-default value, but those users will already be encountering some pretty broken UI elsewhere in the editor, so this is comparatively insignificant.
Anyway, I pushed an update!
| * @since 7.1.0 | ||
| * @var array | ||
| */ | ||
| const VIEWPORT_BREAKPOINTS = array( |
There was a problem hiding this comment.
Maybe DEFAULT_VIEWPORT_BREAKPOINTS to make sure?
| $viewport_breakpoints = WP_Theme_JSON_Gutenberg::get_viewport_breakpoints( $viewport_settings ); | ||
| $viewport_media_queries = array( | ||
| 'mobile' => "@media (width <= {$viewport_breakpoints['mobile']})", | ||
| 'tablet' => "@media ({$viewport_breakpoints['mobile']} < width <= {$viewport_breakpoints['tablet']})", | ||
| 'desktop' => "@media (width > {$viewport_breakpoints['tablet']})", |
There was a problem hiding this comment.
get_viewport_breakpoints() is public because this output only I guess? which is probably fine.
Unless there was a way to return 'desktop' => "@media (width > {$viewport_breakpoints['tablet']})", from get_responsive_media_queries via options or something?
I have in the back of my mind future extraction of all Theme_JSON styles logic to the style engine methods. Too early to untangle all that now though.
There was a problem hiding this comment.
Yeah, agree with this, I think it should use get_responsive_media_queries.
The function could have options for defining output, or the other code could omit desktop after getting the options.
I kind of prefer the latter because it means there's no additions to the public API. I guess it'd just be unset( $media_queries[ 'desktop' ] ).
There was a problem hiding this comment.
you mean passing an option to not output desktop, and outputting it by default? you'd still have options as a function param though?
There was a problem hiding this comment.
I dunno i think I prefer the adding the option to output desktop. In most cases we won't need it, and it'll be a pain to have to remember to always unset the desktop value 😅
| $css_rules = array(); | ||
| $supported_pseudo_states = WP_Theme_JSON_Gutenberg::VALID_BLOCK_PSEUDO_SELECTORS[ $block_name ] ?? array(); | ||
| $style = $block['attrs']['style'] ?? array(); | ||
| $css_rules = array(); |
There was a problem hiding this comment.
How I hate this = alignment lint rule. 😆
Ok, "hide whitespace" now active for the rest of the review.
| if ( ! static::is_valid_viewport_breakpoint_order( $breakpoints ) ) { | ||
| return static::VIEWPORT_BREAKPOINTS; | ||
| } |
There was a problem hiding this comment.
Personally I'm not sure we need to bother checking this. Could we instead leave it up to the author to get it right? Alternatively, the code could output a warning. I'm not sure if there's precedence for theme.json warnings.
How it works now, ignoring the provided values and silently using the default values feels like the wrong choice to me, the code should have some kind of feedback loop about what to fix instead of silence.
There was a problem hiding this comment.
yeha leaving it up to the author is better I reckon, and we already do it for mixed unit values so might as well do it all the time.
| /** | ||
| * Returns viewport breakpoint sizes. | ||
| * | ||
| * @since 7.1.0 | ||
| * | ||
| * @param array|null $viewport_settings Viewport settings from theme.json. | ||
| * @return array Viewport breakpoint sizes. | ||
| */ |
There was a problem hiding this comment.
The docblock is a little bit bare minimum (others in this file too). The code does more than is mentioned which might be useful for others to know.
| foreach ( array_keys( static::VIEWPORT_BREAKPOINTS ) as $breakpoint ) { | ||
| if ( | ||
| isset( $viewport_settings[ $breakpoint ] ) && | ||
| static::is_valid_viewport_breakpoint_size( $viewport_settings[ $breakpoint ] ) |
There was a problem hiding this comment.
Probably similar here that it could return a warning, but agree that it's better here to reject a value.
| * @param mixed $viewport_settings Viewport settings from theme.json. | ||
| * @return array Sanitized viewport settings. | ||
| */ | ||
| private static function sanitize_viewport_settings( $viewport_settings ) { |
There was a problem hiding this comment.
This seems to duplicate a lot of what get_viewport_breakpoints is doing. Maybe get_viewport_breakpoints can call sanitize_viewport_settings instead and avoid doing the work itself? The defaults and valid settings then need only be merged.
There's probably a few other ways to structure this as well. I realize that sometimes get_viewport_breakpoints is probably already working on sanitized values.
| foreach ( $valid_block_names as $block ) { | ||
| $schema_settings_blocks[ $block ] = static::VALID_SETTINGS; | ||
| $schema_settings_blocks[ $block ] = static::VALID_SETTINGS; | ||
| unset( $schema_settings_blocks[ $block ]['viewport'] ); |
There was a problem hiding this comment.
I guess it doesn't necessarily need improving in this PR, but every block has the same schema by the looks of it, so perhaps it should be possible to generate it only once instead of in the foreach loop.
| protected static function remove_insecure_element_styles( $elements, $responsive_media_queries = null ) { | ||
| $sanitized = array(); | ||
| $valid_element_names = array_keys( static::ELEMENTS ); | ||
| $responsive_media_queries = $responsive_media_queries ?? static::get_responsive_media_queries(); |
There was a problem hiding this comment.
Is there a case where $responsive_media_queries isn't passed in? Maybe the call to static::get_responsive_media_queries() isn't needed.
There was a problem hiding this comment.
I don't think so as of now. Maybe it's not worth doing this as a hypothetical protection against future uses 😅
There was a problem hiding this comment.
I removed the call and wrapped the bit that processes $responsive_media_queries in a condition to avoid explosions if any extension of the class is calling either of the functions with only one param (they've both been in since 6.8).
| $sanitized = array(); | ||
| protected static function remove_insecure_inner_block_styles( $blocks, $responsive_media_queries = null ) { | ||
| $sanitized = array(); | ||
| $responsive_media_queries = $responsive_media_queries ?? static::get_responsive_media_queries(); |
| const viewportBreakpoints = getViewportBreakpoints( viewportSettings ); | ||
| const isMobileViewport = useMediaQuery( | ||
| `(width <= ${ viewportBreakpoints.mobile })`, | ||
| view | ||
| ); | ||
| const isTabletViewport = useMediaQuery( | ||
| `(${ viewportBreakpoints.mobile } < width <= ${ viewportBreakpoints.tablet })`, | ||
| view | ||
| ); |
There was a problem hiding this comment.
It's a shame this can't use getResponsiveMediaQueries instead, but there's slightly different syntax. This feels quite manual having to assemble the string.
| const blockVisibility = attributes?.metadata?.blockVisibility; | ||
| const deviceType = | ||
| settings?.[ deviceTypeKey ]?.toLowerCase() || 'desktop'; | ||
| const viewportSettings = settings?.__experimentalFeatures?.viewport; |
There was a problem hiding this comment.
Feels strange that it has to use __experimentalFeatures all the time. 🤔
There was a problem hiding this comment.
that's apparently the default place to add style-related settings 🤷 I'm not sure how feasible it would be to restructure.
40efcb2 to
c182458
Compare
|
Thanks for the reviews folks! I think I've addressed all the feedback now. |
What?
Part of #75707
Adds a viewport object to theme.json settings, with two child values:
tabletandmobile. These can be set topx,emorremvalues.This looks like a big changeset but the important part is the additions to the theme json class because that's where the viewport values are processed; the changes in the other files are basically fetching those values from theme.json to replace the hardcoded ones we had before.
Testing Instructions
theme.json, add the following in the settings object:Use of AI Tools
Used codex/gpt 5.5 to write and iterate on the code. hand reviewed by me.