Block editor canvas iframe: play around with restricting max height of iframe #58484
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
- pass block editor canvas container element properties (height) down to iframe. For this experiment just pass container height. - Use container height as the max height of the iframe. - Set maxheight of iframe when in iframe zoom mode to avoid iframe stretching where blocks have vh set - Turn off unhelpful transition animations - Use container height as the max height of the iframe. - Set maxheight of iframe when in iframe zoom mode to avoid iframe stretching where blocks have vh set - Turn off unhelpful transition animations
ramonjd
commented
Jan 31, 2024
| className: classnames( | ||
| 'edit-site-visual-editor__editor-canvas', | ||
| { | ||
| 'is-focused': isFocused && canvasMode === 'view', | ||
| } | ||
| ), | ||
| maxHeight: | ||
| isZoomOutMode && containerHeight |
There was a problem hiding this comment.
I guess we wouldn't want to set a maxHeight in isZoomOutMode always.
Right now I'm just testing in the site editor when browsing styles.
|
Size Change: +122 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
revert unintended change to frameSize
ramonjd
commented
Jan 31, 2024
| @@ -59,12 +63,25 @@ export default function SiteEditorCanvas() { | |||
| const isTemplateTypeNavigation = templateType === NAVIGATION_POST_TYPE; | |||
| const isNavigationFocusMode = isTemplateTypeNavigation && isFocusMode; | |||
| const forceFullHeight = isNavigationFocusMode; | |||
| const [ containerHeight, setContainerHeight ] = useState(); | |||
| const containerRef = useRefEffect( | |||
There was a problem hiding this comment.
I expect this is not ideal.
It's a lot of faffery just to passing down the iframe container's height.
Maybe we pass down the ref? 🤷🏻
ramonjd
commented
Jan 31, 2024
| // const maxHeight = props.style?.maxHeight || 0; | ||
| const maxContentHeight = | ||
| maxHeight && contentHeight > maxHeight | ||
| ? maxHeight / ( 1 - scale ) |
There was a problem hiding this comment.
Wondering if we could get the container height using DOM API here instead 🤷🏻
Would save all the ref passing and other chicanery
|
In #59334, html now scales instead of the iframe, so the problem this PR is trying to solve may no longer occur. |
|
Thanks @t-hamano I'd forgotten about this PR 😄 You're right: it looks much better. 2024-02-28.15.49.40.mp4 |
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.
An experiment only:
Some example HTML
Before
2024-01-31.16.09.59.mp4
After
2024-01-31.16.08.41.mp4
What?
Why?
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast