Zoom Out: Bundle behavior in block-editor and add story #66240
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
youknowriad
added
[Type] Code Quality
youknowriad
requested review from
draganescu,
getdave,
jeryj,
richtabor,
ellatrix and
ntsekouras
|
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. |
youknowriad
commented
Oct 18, 2024
| scale: 'default', | ||
| frameSize: '40px', | ||
| } | ||
| : {}; |
There was a problem hiding this comment.
This is the main change in this PR, basically the zoom-out behavior is absorbed into the block-editor.
youknowriad
commented
Oct 18, 2024
| @@ -61,7 +56,6 @@ function DocumentTools( { className, disableBlockTools = false } ) { | |||
| showIconLabels: get( 'core', 'showIconLabels' ), | |||
| isDistractionFree: get( 'core', 'distractionFree' ), | |||
| isVisualMode: getEditorMode() === 'visual', | |||
| isZoomedOutView: isZoomOut(), | |||
There was a problem hiding this comment.
This is just a small cleanup, a value that was not used.
youknowriad
commented
Oct 18, 2024
| const { unlock } = __dangerousOptInToUnstableAPIsOnlyForCoreModules( | ||
| 'I acknowledge private features are not for use in themes or plugins and doing so will break in the next version of WordPress.', | ||
| '@wordpress/edit-site' | ||
| ); |
There was a problem hiding this comment.
Ideally setZoomLevel shouldn't be a private API, I think a block-editor setting makes sense for the zoom level setting but I'm keeping that separate.
Also, it seems the value of zoom-level doesn't have any impact, it should probably correspond to the applied scale.
youknowriad
commented
Oct 18, 2024
| > | ||
| <EnableZoomOut /> | ||
| <BlockCanvas height="500px" styles={ editorStyles }> | ||
| <style>{ contentCss }</style> |
There was a problem hiding this comment.
This highlighted that third-party block editors need to load the "content.css" file from the block-editor package within the iframe. I would love if this was not necessary (and absorbed by the component automatically instead). cc @ellatrix in case you have ideas.
There was a problem hiding this comment.
Not sure I'm following. Shouldn't it be merged into editorStyles?
There was a problem hiding this comment.
@ellatrix No, editorStyles seem to be rewritten to add the wrapper... and I think that breaks some of the "content" styles that we provide.
@ntsekouras not related to these styles at all, because the styles here are the "content" styles that should apply within the iframe, the rest is just editor styles, ui styles...
There was a problem hiding this comment.
@youknowriad What does that break? Ideally, everything should receive the content wrapper. For whatever styles that it is not the case now it will have to be redone at some point. It shouldn't break the styles -- it should merely add the prefix that adds zero specificity.
There was a problem hiding this comment.
- That's weird, I remember removing
wp-editorand here it is indeed: Styles: remove wp-editor from wp-edit-blocks dependencies #33496. It probably wasn't backported. I'll create a backport PR. - Looking at reusable blocks, this only contains a single rule that is for a modal (so nothing in the content). I'll remove it, and also backport it (in the same PR since we're in RC anyway).
- Patterns also seems to be for modals and a button. Could be that that button is part of the placeholder, I'll check.
Generally for UI, it would simplify a lot if we used shadow DOM so we don't need components and UI styling at all.
There was a problem hiding this comment.
Here's the one for reusable blocks: #66285.
There was a problem hiding this comment.
The backport for wp-editor removal: WordPress/wordpress-develop#7609
There was a problem hiding this comment.
Here's the removal of the patterns (UI) stylesheet: #66306. It's indeed all UI related.
There was a problem hiding this comment.
One the questions is still: how would we load the components (for now) and block library styles directly from inside the block-editor package? Do we "raw" import the CSS or something?
youknowriad
commented
Oct 18, 2024
| <div | ||
| className="editor-zoom-out" | ||
| onKeyDown={ ( event ) => event.stopPropagation() } | ||
| style={ { background: '#ddd', border: '1px solid gray' } } |
There was a problem hiding this comment.
The background style shouldn't be necessary, at the moment when the scaling is applied, the "iframe" content is scaled but there's still a "white" background around the iframe content, I believe this style should be bundled within the block-editor package and the iframe should have the same gray background applied within the iframe.
youknowriad
commented
Oct 18, 2024
| @@ -0,0 +1,39 @@ | |||
| export const pattern = `<!-- wp:cover {"customOverlayColor":"#eb4c77","contentPosition":"center center","isDark":false,"align":"full","style":{"spacing":{"padding":{"top":"6vw","right":"6vw","bottom":"6vw","left":"6vw"},"margin":{"top":"0"}}}} --> | |||
There was a problem hiding this comment.
This is just a random pattern I took from the pattern library to show case the zoom-out in storybook.
|
Size Change: +13 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 03d8b4d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11405092796
|
ntsekouras
approved these changes
Oct 21, 2024
karthick-murugan
pushed a commit
to karthick-murugan/gutenberg
that referenced
this pull request
Nov 13, 2024
) Co-authored-by: youknowriad <[email protected]> Co-authored-by: ntsekouras <[email protected]> Co-authored-by: ellatrix <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
While trying zoom-out in a personal experiment, I noticed that to be able to enable Zoom-out, setting the zoom-level is not enough, you have to also pass some extra arguments to the iframe.
These extra things shouldn't be needed and the canvas should be smart enough to adapt directly.
This PR solves that and also adds a storybook story in order to be able to check that third-part block editors can enable zoom-out without too much hassle.
Testing Instructions
1- Check zoom-out scaling
2- Run storybook
npm run storybook:devand check that the "zoom-out" story scales properly.