-
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
Resizable editor experiment #19082
Resizable editor experiment #19082
Changes from all commits
dfaa1b7
2720b04
6f0ca43
600a9ff
122ccee
75f7d28
812e31a
39370bc
a9f5d54
21f4a16
63bbfa5
aaec420
ad4c669
86484be
6dd6e75
3f70996
e8e1d92
f56765e
93e6e45
1e446e0
860670d
4ecef11
d23bac2
9c42b28
3544005
a6668be
51e578b
c28e1e6
231a89f
718f246
f832790
7d2bf12
50370fc
61471d0
0696f6a
dd53ad9
aedf6bc
3356171
ba531c4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -0,0 +1,128 @@ | ||||
/** | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd expect if the default export of this file is Prior art: |
||||
* External dependencies | ||||
*/ | ||||
import { filter, get } from 'lodash'; | ||||
import { match } from 'css-mediaquery'; | ||||
|
||||
/** | ||||
* WordPress dependencies | ||||
*/ | ||||
import { useEffect } from '@wordpress/element'; | ||||
|
||||
const ENABLED_MEDIA_QUERY = '(min-width:0px)'; | ||||
const DISABLED_MEDIA_QUERY = '(min-width:999999px)'; | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Uh, I guess we could add a few more digits. But I'd rather leave it as is just because I'd love to see the screenshot for that bug report 😂 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I concur. |
||||
|
||||
const VALID_MEDIA_QUERY_REGEX = /\((min|max)-width:[^\(]*?\)/g; | ||||
|
||||
function getStyleSheetsThatMatchHostname() { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reasons to return early from this function if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. This code shouldn't ever run on the server side, but better safe than sorry I guess 😅 |
||||
if ( ! window ) { | ||||
return; | ||||
} | ||||
return filter( | ||||
get( window, [ 'document', 'styleSheets' ], [] ), | ||||
( styleSheet ) => { | ||||
return ( | ||||
styleSheet.href && | ||||
styleSheet.href.includes( window.location.hostname ) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a couple of issues with this simplistic of logic:
|
||||
); | ||||
} | ||||
); | ||||
} | ||||
|
||||
function isReplaceableMediaRule( rule ) { | ||||
if ( ! rule.media ) { | ||||
return false; | ||||
} | ||||
// Need to use "media.mediaText" instead of "conditionText" for IE support. | ||||
return !! rule.media.mediaText.match( VALID_MEDIA_QUERY_REGEX ); | ||||
} | ||||
|
||||
function replaceRule( styleSheet, newRuleText, index ) { | ||||
styleSheet.deleteRule( index ); | ||||
styleSheet.insertRule( newRuleText, index ); | ||||
} | ||||
|
||||
function replaceMediaQueryWithWidthEvaluation( ruleText, widthValue ) { | ||||
return ruleText.replace( VALID_MEDIA_QUERY_REGEX, ( matchedSubstring ) => { | ||||
if ( | ||||
match( matchedSubstring, { | ||||
type: 'screen', | ||||
width: widthValue, | ||||
} ) | ||||
) { | ||||
return ENABLED_MEDIA_QUERY; | ||||
} | ||||
return DISABLED_MEDIA_QUERY; | ||||
} ); | ||||
} | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is really cool! 😻 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Props to @jorgefilipecosta for most of the replacing logic 😁 |
||||
|
||||
/** | ||||
* Function that manipulates media queries from stylesheets to simulate a given viewport width. | ||||
* | ||||
* @param {string} marker CSS selector string defining start and end of manipulable styles. | ||||
* @param {number} width Viewport width to simulate. | ||||
*/ | ||||
export default function useSimulatedMediaQuery( marker, width ) { | ||||
useEffect( () => { | ||||
const styleSheets = getStyleSheetsThatMatchHostname(); | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As far as I can tell, we're pulling stylesheets anew every single time this runs which is unnecessary. I think this could be wrapped in |
||||
const originalStyles = []; | ||||
styleSheets.forEach( ( styleSheet, styleSheetIndex ) => { | ||||
Comment on lines
+67
to
+69
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
...at which point, (Aside: TypeScript checking as in #18838 could have caught this) |
||||
let relevantSection = false; | ||||
for ( | ||||
let ruleIndex = 0; | ||||
ruleIndex < styleSheet.cssRules.length; | ||||
++ruleIndex | ||||
) { | ||||
const rule = styleSheet.cssRules[ ruleIndex ]; | ||||
if ( | ||||
! relevantSection && | ||||
!! rule.cssText.match( new RegExp( `#start-${ marker }` ) ) | ||||
) { | ||||
relevantSection = true; | ||||
} | ||||
|
||||
if ( | ||||
relevantSection && | ||||
!! rule.cssText.match( new RegExp( `#end-${ marker }` ) ) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This logic is moderately useful for Gutenberg-written stylesheets but untenable for theme and plugin stylesheets. As an example, I'm currently working on porting a client over to Gutenberg and they have 200+ individual SCSS files that are used in the theme. Adding this rule that makes no sense in any other context to a stylesheet is polluting and doesn't make sense for the other 10 teams working on the site. I would propose that a better rule would be to look for any rules with |
||||
) { | ||||
relevantSection = false; | ||||
} | ||||
|
||||
if ( ! relevantSection || ! isReplaceableMediaRule( rule ) ) { | ||||
continue; | ||||
} | ||||
const ruleText = rule.cssText; | ||||
if ( ! originalStyles[ styleSheetIndex ] ) { | ||||
originalStyles[ styleSheetIndex ] = []; | ||||
} | ||||
originalStyles[ styleSheetIndex ][ ruleIndex ] = ruleText; | ||||
replaceRule( | ||||
styleSheet, | ||||
replaceMediaQueryWithWidthEvaluation( ruleText, width ), | ||||
ruleIndex | ||||
); | ||||
} | ||||
} ); | ||||
return () => { | ||||
originalStyles.forEach( ( rulesCollection, styleSheetIndex ) => { | ||||
if ( ! rulesCollection ) { | ||||
return; | ||||
} | ||||
for ( | ||||
let ruleIndex = 0; | ||||
ruleIndex < rulesCollection.length; | ||||
++ruleIndex | ||||
) { | ||||
const originalRuleText = rulesCollection[ ruleIndex ]; | ||||
if ( originalRuleText ) { | ||||
replaceRule( | ||||
styleSheets[ styleSheetIndex ], | ||||
originalRuleText, | ||||
ruleIndex | ||||
); | ||||
} | ||||
} | ||||
} ); | ||||
}; | ||||
}, [ width ] ); | ||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,10 @@ | ||
// This tag marks the start of the styles that apply to editing canvas contents and need to be manipulated when we resize the editor. | ||
#start-resizable-editor-section { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jorgefilipecosta Can we use the simulated queries introduced here in the customizer sidebar? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I think it would work as expected in the customizer view we would useSimulatedMediaQuery with a value that forces a mobile-like view. It will probably not work magically right away and we will probably need some adjustments to make things look well on the customizer like I did in #17960. |
||
display: none; | ||
} | ||
|
||
// Only add styles for components that are used inside the editing canvas here: | ||
|
||
@import "./components/block-icon/style.scss"; | ||
@import "./components/block-inspector/style.scss"; | ||
@import "./components/block-list/style.scss"; | ||
|
@@ -19,11 +26,9 @@ | |
@import "./components/colors-gradients/style.scss"; | ||
@import "./components/contrast-checker/style.scss"; | ||
@import "./components/default-block-appender/style.scss"; | ||
@import "./components/editor-skeleton/style.scss"; | ||
@import "./components/link-control/style.scss"; | ||
@import "./components/image-size-control/style.scss"; | ||
@import "./components/inner-blocks/style.scss"; | ||
@import "./components/inserter/style.scss"; | ||
@import "./components/inserter-list-item/style.scss"; | ||
@import "./components/media-replace-flow/style.scss"; | ||
@import "./components/media-placeholder/style.scss"; | ||
|
@@ -39,3 +44,13 @@ | |
@import "./components/warning/style.scss"; | ||
@import "./components/writing-flow/style.scss"; | ||
@import "./hooks/anchor.scss"; | ||
|
||
// This tag marks the end of the styles that apply to editing canvas contents and need to be manipulated when we resize the editor. | ||
#end-resizable-editor-section { | ||
display: none; | ||
} | ||
|
||
// Styles for components that are used outside of the editing canvas go here: | ||
|
||
@import "./components/editor-skeleton/style.scss"; | ||
@import "./components/inserter/style.scss"; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,8 @@ | ||
// This tag marks the start of the styles that apply to editing canvas contents and need to be manipulated when we resize the editor. | ||
#start-resizable-editor-section { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need comments on these? |
||
display: none; | ||
} | ||
|
||
@import "./archives/editor.scss"; | ||
@import "./audio/editor.scss"; | ||
@import "./block/editor.scss"; | ||
|
@@ -57,3 +62,8 @@ | |
margin-top: $default-block-margin; | ||
margin-bottom: $default-block-margin; | ||
} | ||
|
||
// This tag marks the end of the styles that apply to editing canvas contents and need to be manipulated when we resize the editor. | ||
#end-resizable-editor-section { | ||
display: 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.
What about viewports which are narrower than 400px?
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.
Whoops, that needs to be in a media query.