-
Notifications
You must be signed in to change notification settings - Fork 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
Modify Block Pattern Preview Viewport #43362
Conversation
This PR does not affect the size of JS and CSS bundles shipped to the user's browser. Generated by performance advisor bot at iscalypsofastyet.com. |
Caution: This PR affects files in the FSE Plugin on WordPress.com D45031-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing in the FSE Plugin for more info: PCYsg-ly5-p2 |
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.
Let me know if the testing instructions need more clarification.
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.
Nice! I think pattern designers need to definitely start adding a viewportWidth
value that makes sense for the specific pattern (they probably all shouldn't be the same value).
Taking a quick look at some of the core patterns viewport width settings:
- A handful seem to be set at
500
- A couple set at
1000
- A bunch are still
undefined
It looks like the default for undefined
is 700
:
https://github.com/WordPress/gutenberg/blob/9c256e3ae8bb715a9a863e7081df479acde71666/packages/block-editor/src/components/block-preview/index.js#L22
Since all of these were previously undefined, we have jumped from 700
to 1280
. 1280
may be unfittingly wide to view some of the smaller scoped patterns. (The largest I saw in the core list was 1000
, but that doesn't mean 1280
couldn't be a practical value for some patterns.)
We may want to set these values differently on an individual basis, but will probably need some collaboration with design to figure out which values make sense for which patterns. 🤔 Maybe if we go with supporting 3 sizes (500, 700, 1000) for general cases, dividing the current list up into those groups shouldn't bee too bad to figure out?
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 think pattern designers need to definitely start adding a viewportWidth value that makes sense for the specific pattern (they probably all shouldn't be the same value).
We may want to set these values differently on an individual basis, but will probably need some collaboration with design to figure out which values make sense for which patterns.
Great observations. I totally agree 👍
@iamtakashi had suggested trying 1280 with all patterns, but I expect those numbers to change as well.
@jeyip Thanks for working on this! The patterns look much better in the preview now. There have been new patterns added and shipped with FSE 1.10.0. Can you also do the same thing for those patterns? I've tested with a Simple site in the 4 browsers mentioned above. And it mostly looked good. The only thing that I've noticed was the |
I'm on it!
I'll take a look at the HTML markup used for the preview. @iamtakashi Would you mind sending a screenshot of |
I was poking around for a bit regarding this and I am a bit confused as to why this might be as well. Just looking at how other patterns change with I notice these bullet points to the left of the images in the editor (but not on the reference @ianstewart provided above): If these are not intended, maybe they could be the culprit for the unintended spacing in the preview? 🤔 |
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.
Good observations @Addison-Stavlo. I'll keep the bullet points in mind as I revisit this PR. I actually thought this issue might also be semi-related. It looks like some block pattern preview styling isn't being applied properly
1e45a01
to
040bbce
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.
It looks like this issue might have already existed in production, prior to these updates.
It definitely existed previously. I think the expectation was that expanding the viewportWidth to 1280 would fix this issue. Oddly, it has not for some other reasons.
Everything else seems to look/behave well. It sounds like everything else looks good to folks, so I think we can move forward here.
How about we create another issue for the mis-behaving pattern and add it to the board?
That sounds good idea to me. This specific issue with the collage block warrants a lot more discussion, but I agree that it shouldn't block this PR.
I see! That makes sense. Thanks for clarifying @Addison-Stavlo After some digging, I determined that the cause is a missing Here's a link to the actual styles: Some remaining questions I have that can be addressed when we create a new issue:
The alternative is to add box-sizing as an inline style to block pattern previews. That, however, seems susceptible to problems in the future if CSS styles continue to diverge. |
Let me know if there are any concerns. Otherwise, I'll go with Addison's suggestion, merge these changes tomorrow, and create a separate issue for the collage block. |
@jeyip @Addison-Stavlo Thank you both for looking into the issue with the preview with the collage gallery block. Yes, I agree. It makes sense to tackle the issue separately and merge these changes! |
I also agree that the preview and the editor should be as close as possible. Maybe, it'd be best to report & contribute a change in Gutenberg plugin repo unless this is a dotcom specific issue? |
Im assuming its not deliberate that the inserter previews are missing the box-sizing style, but maybe there is more history there I am unaware of. I wonder if @youknowriad or @nosolosw may have more insight here (or know of someone who might?), but we should probably open an issue in core regarding the lack of style parity (#43362 (comment)) between previews and editor. A side note for clarity / future info - the term 'global styles' is often used to refer to a user facing system to apply font types, sizes, etc. globally across a users website. But I think its clear that we aren't currently talking about that here. 😄 |
Waiting for the Calypso queue to clear up before merging |
040bbce
to
8eff07d
Compare
I created a separate issue in core Gutenberg for the |
8eff07d
to
938bb02
Compare
Changes proposed in this Pull Request
Fixes #42165. Increase viewport width of block pattern previews for better preview scaling
Screenshots
Before Example
After Example
Testing
Instructions
cd apps/full-site-editing; yarn dev --sync
More info in PCYsg-ly5-p2 #build-scriptsRequirements
Browsers