-
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
Add simulate media queries #17946
Add simulate media queries #17946
Conversation
Related: #13203 |
//'block-editor/style.css', | ||
'block-library/style.css', | ||
'block-library/theme.css', | ||
'block-library/editor.css', | ||
//'format-library/style.css', |
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.
Why are 2 commented out? Should we just remove them?
packages/edit-post/src/components/sidebar/settings-sidebar/index.js
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/sidebar/settings-sidebar/index.js
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/sidebar/settings-sidebar/index.js
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/sidebar/settings-sidebar/index.js
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/sidebar/settings-sidebar/index.js
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/sidebar/settings-sidebar/index.js
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/sidebar/settings-sidebar/index.js
Outdated
Show resolved
Hide resolved
} ); | ||
}; | ||
}, | ||
[ partialPaths, value ] |
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.
As an optimization, we could pass in a set of values for which we can precompute class name rules for, for each media query. Like in this CodePen after these fixes.
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.
Hi @epiqueras, yes we could do an optimization where we precompute the media query evaluation for a predefined set of values.
I think we can also have big performance wins by just parsing the media query rules one time at the start, and then when we change the simulated width we can just compare the new width with the media query value that were already parsed.
I did not invest much in performance yet as we did not decide yet if we want to follow this idea. But in my tests even without optimizations, I don't notice any delay with changing the simulated width.
Hi @epiqueras, your suggestions were applied 👍 Let me know what you think of this PR and if you think it is something we should follow. |
I like the approach. @aduth you've also been looking at this. What do you think? |
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 seems promising 👍 I think the challenge, depending how accurate we want the simulation to be, will be the actual evaluation of the media queries, and in accounting for all relevant stylesheets which might have an impact on the preview.
@@ -22,6 +23,45 @@ import PageAttributes from '../page-attributes'; | |||
import MetaBoxes from '../../meta-boxes'; | |||
import PluginDocumentSettingPanel from '../plugin-document-setting-panel'; | |||
|
|||
const PARTIAL_PATHS = [ |
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.
How do you imagine we account for styles associated to using editor_style
or style
in register_block_type
?
If we can have the absolute paths for those stylesheets, I assume it wouldn't be much more of a stretch to try to also have the full paths for the editor, block library, and theme styles. The idea of hard-coding a few partial paths doesn't seem like it would be a very effective solution.
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.
That's one of the hardest challenges of this approach -- deciding what stylesheets should be converted.
The idea of hard-coding a few partial paths doesn't seem like it would be a very effective solution.
I agree. I used hard codding of these styles as a simpler way to test the approach. My idea is for the server to pass a setting named "media_query_rewrite_paths" containing the paths of style sheets that should be rewritten. I guess some of these styles will be common to the styles we use in for the editor styles wrapping rewrite.
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.
Currently these only exist as individual stylesheets in the plugin though. In core they're all bundled together with styles that we don't want to change at all such as wp-admin, editor and component ones. So the solution will need to include making sure all our post-content-relevant styles (or all our full-site-editing-relevant styles?) are bundled as a separate stylesheet in core, and then we'll probably also want to include the theme stylesheet.
} | ||
} ); | ||
return () => { | ||
originalStyles.forEach( ( rulesCollection, styleSheetIndex ) => { |
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 had trouble thinking in my experiment how to reliably restore styles after the simulation is complete. I think this combination of useEffect
plus a consistent reference to both the original styleSheets
and originalStyles
should work pretty well.
I suppose there might be the off chance that if a specific rule in a stylesheet were dynamically modified elsewhere, the ruleIndex
might become inaccurate at the time we restore styles. That seems like an edge case though.
packages/block-editor/src/components/simulate-media-query/index.js
Outdated
Show resolved
Hide resolved
} | ||
|
||
function replaceMediaQueryWithWidthEvaluation( ruleText, widthValue ) { | ||
return ruleText.replace( VALID_MEDIA_QUERY_REGEX, ( match, minOrMax, value ) => { |
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 would be nice if we could actually evaluate the media query, rather than trying to extract a pixel value. The currnet implementation would miss a bunch of alternate units or complex queries.
I explored this a bit at #13203 (comment), with some approaches using iframe
or an npm package.
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 current implementation is very naive. But we can easily expand the replace mechanism to support complex queries e.g: screen and (min-width: 40em)
is replaced with screen and (min-width: 999999px)
, we would need some logic to convert units which is not hard.
Evaluating or using a package is interesting if we want to support more complex scenarios like (hover: none)
to simulate touch. The npm package would be very interesting if it relied on browser evaluation for properties not explicitly passed.
e.g:
cssMediaquery.match('screen and (min-width: 40px)', {
width: '1024px'
});
Returns false because we did not pass "type : 'screen'", while in a replace approach it may be true because the current device is a screen. I guess given that defaults are not assumed it will not be easy to provide a complete set of default values.
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 npm package would be very interesting if it relied on browser evaluation for properties not explicitly passed.
Are we interested in any type other than 'screen'?. If we pass the match function type: 'screen'
it'll match all media queries that don't explicitly have a different type set, so
cssMediaquery.match('(min-width: 40px)', {
width: '1024px',
type: 'screen'
});
will return true.
Not quite sure how it handles things like prefers-reduced-motion
though.
packages/block-editor/src/components/simulate-media-query/index.js
Outdated
Show resolved
Hide resolved
ffdb0c0
to
3e588f6
Compare
…ex.js Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
…ex.js Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
…ex.js Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
…ex.js Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
…ex.js Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
…ex.js Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
…ex.js Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
3e588f6
to
a4640f0
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 is working better than my experiment in #19082 so I'm going to merge this branch with mine and have a play with it 😄
} | ||
|
||
function replaceMediaQueryWithWidthEvaluation( ruleText, widthValue ) { | ||
return ruleText.replace( VALID_MEDIA_QUERY_REGEX, ( match, minOrMax, value ) => { |
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 npm package would be very interesting if it relied on browser evaluation for properties not explicitly passed.
Are we interested in any type other than 'screen'?. If we pass the match function type: 'screen'
it'll match all media queries that don't explicitly have a different type set, so
cssMediaquery.match('(min-width: 40px)', {
width: '1024px',
type: 'screen'
});
will return true.
Not quite sure how it handles things like prefers-reduced-motion
though.
@@ -22,6 +23,45 @@ import PageAttributes from '../page-attributes'; | |||
import MetaBoxes from '../../meta-boxes'; | |||
import PluginDocumentSettingPanel from '../plugin-document-setting-panel'; | |||
|
|||
const PARTIAL_PATHS = [ |
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.
Currently these only exist as individual stylesheets in the plugin though. In core they're all bundled together with styles that we don't want to change at all such as wp-admin, editor and component ones. So the solution will need to include making sure all our post-content-relevant styles (or all our full-site-editing-relevant styles?) are bundled as a separate stylesheet in core, and then we'll probably also want to include the theme stylesheet.
Closing this PR as @tellthemachines already mixed the concept being proposed here with the work in PR #19082 that also includes a UI. Thank you for joining this effort @tellthemachines 👍 |
Description
This PR adds an experimental block-editor component that allows one to simulate a width value in media queries part of a specific set.
This together with PR #17085 that adds the same simulation mechanism for withViewPortMatch will allow us to show a mobile UI in the block editor present in the customizer even though the window size may be big.
It will also allow us to provide components that allow one to switch between mobile desktop and target views.
To force media queries to react as if the width as different from the true window size we iterate on the stylesheets and each time if find a media query with a min or max-width rule evaluate that value against our simulated value if the rule matches we replace the media query with another media query that evaluates to true, if the rule does not match we replace the media-query with a rule that evaluates to false.
This PR is an alternative to #17057.
It fixes the biggest problems:
In this case, the performance is very hight we just change the media queries when the simulated value change and then we totally rely on the browser and the specificity is not changed.
How has this been tested?
I added media & text block which has a stack on mobile option.
I used the test UI added on this PR (to be removed before the merge) that allows us to change the simulated value or to disable the simulation and I verified I was able to trigger to stack on mobile behavior even though the window was big.