Editor: Unify device preview styles #56904
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
the
[Type] Code Quality
youknowriad
requested review from
getdave,
jameskoster,
jasmussen and
stokesman
|
Size Change: -216 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in df1360d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7145127382
|
youknowriad
force-pushed
the
update/simplify-device-previews
branch
from
2877fcf
to
447a8c6
Compare
youknowriad
requested review from
ajitbohra,
ntwb,
nerrad,
ellatrix,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
youknowriad
force-pushed
the
move/edit-template-notification
branch
from
1bd7b7e
to
5b874bf
Compare
ellatrix
reviewed
Dec 8, 2023
| @@ -361,6 +372,7 @@ function EditorCanvas( | |||
| } | |||
| renderAppender={ renderAppender } | |||
| /> | |||
| <EditTemplateBlocksNotification contentRef={ localRef } /> | |||
There was a problem hiding this comment.
Wasn't this also in the other PR?
There was a problem hiding this comment.
yeah, this is based on the other one :)
youknowriad
force-pushed
the
update/simplify-device-previews
branch
from
447a8c6
to
df1360d
Compare
ellatrix
reviewed
Dec 8, 2023
| hasFixedToolbar: | ||
| isFeatureActive( 'fixedToolbar' ) || | ||
| getDeviceType() !== 'Desktop', | ||
| hasFixedToolbar: isFeatureActive( 'fixedToolbar' ), |
There was a problem hiding this comment.
Don't we still need this? Although I see the floating toolbar even in trunk for the post editor...
There was a problem hiding this comment.
Yeah, it's a left over for a very old behavior.
anomiex
added a commit
to Automattic/wp-calypso
that referenced
this pull request
Jan 5, 2024
* WordPress/gutenberg#56921 changed the label of the preview button from "Preview" to "View". * WordPress/gutenberg#56904 removed the setting of an `is-xxx-preview` class when using the in-page preview. Targeting the parent element (that has a static class) instead. * #86033 attempted to fix a test for another Gutenberg change, but the selector used there matches multiple elements in one of our tests and so causes it to fail. Adding `[role="tab"]` into the selector fixes that.
7 tasks
sdixon194
pushed a commit
to Automattic/wp-calypso
that referenced
this pull request
Jan 5, 2024
* WordPress/gutenberg#56921 changed the label of the preview button from "Preview" to "View". * WordPress/gutenberg#56904 removed the setting of an `is-xxx-preview` class when using the in-page preview. Targeting the parent element (that has a static class) instead. * #86033 attempted to fix a test for another Gutenberg change, but the selector used there matches multiple elements in one of our tests and so causes it to fail. Adding `[role="tab"]` into the selector fixes that.
ellatrix
reviewed
Apr 4, 2024
| @@ -348,6 +357,7 @@ function EditorCanvas( | |||
| <BlockList | |||
| className={ classnames( | |||
| className, | |||
| 'is-' + deviceType.toLowerCase() + '-preview', | |||
There was a problem hiding this comment.
@youknowriad I got a ping from @sergiu-radu that we previous had this class on the .editor-styles-wrapper container, and since 6.5 on the .is-root-container div, so this is a small breaking change. @sergiu-radu, could you maybe give us some more info about why you were relying on this?
There was a problem hiding this comment.
We can consider it adding it to that wrapper too. It would be good to open a detailed issue about this.
There was a problem hiding this comment.
Hi guys 👋
No, this class was not on .editor-styles-wrapper but outside this container...
To be more specific, this class.is-desktop-preview was a direct child container of .edit-post-visual-editor__content-area container and inside it was the .editor-styles-wrapper.
Here is a screenshot - https://share.cleanshot.com/D1h4tF67
Now, in 6.5 the .is-desktop-preview class is placed on .is-root-container and this breaks a lot of things for us, and I think for other classic themes too.
Here is a screenshot - https://share.cleanshot.com/r57zPg57
We will try to make some tests and adjust our code first but if we won't succeed - will open an issue.
Thank you.
There was a problem hiding this comment.
To be clear, per the extensibility guidelines of Gutenberg, this is an internal modifier class. It's unfortunate that theme authors are relying on it. We can try to mitigate the issues though (the class will probably end up being present in two places)
There was a problem hiding this comment.
@youknowriad Why was it needed to move it from outside the canvas to inside the canvas?
There was a problem hiding this comment.
This PR unified the behavior between post and site editors, which also meant some markup simplification (removing unnecessary wrappers in the post editor)
There was a problem hiding this comment.
Hey guys, we made some more investigations and yes, the problem seems to not be related to that class actually but something else (most likely because of 2 scenarios - when we have iframe on desktop and when we don't have the iframe).
We are still investigating this issue and will get back with more details.
Thank you!
There was a problem hiding this comment.
I don't remember exactly but I believe this class was not present at all in the site editor and I just kept it around (for BC) but in a unified place. We can consider moving it to anther place in. EditorCanvas if needed but it shouldn't be something that the editor implementor have to think about. The device handling moved within EditorCanvas
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.
Related #52632
What?
This PR unifies the way "device styles" are applied between the post and site editors. The way these styles were applied was a bit different between both editors. While both editors used iframes for device previews, one of them applied the styles to an extra wrapper and the other applied it to the iframe directly. This PR unifies both and picked the simplest approach (avoid any extra wrapper, so it uses the site editor approach).
The other thing is that the "template mode" had a "padding" in the post editor but no padding in the site editor around the template. This PR unifies that as well and removes the padding.
Testing Instructions
Check devices previews in both post and site editors.