[Block Library - Navigation]: Update example with no mobile view #47508
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
ntsekouras
added
[Package] Block library
ntsekouras
requested review from
getdave,
jameskoster,
jasmussen and
annezazu
|
Size Change: +6 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
|
Thanks for the PR. I'm not sure we should do this. Mostly in principle since the preview should just be real behavior, even if scaled down. In this case it also comes with a couple of other tradeoffs. Two examples, firstly, the dropdown menus don't show, and the capitalization isn't matching:
This may actually be a separate issue, but it's nevertheless an incorrect preview. Secondly, when it's collapsed always in the canvas, it's weird that it isn't collapsed in the preview:
I think solutions to this issue could be to make sure the inline preview is an anchor link to the navigation block as shown inside the style book. Not urgent, but a more generic and generalized solution. |
|
There are some nuances with block examples that are used in previews and I don't have strong opinion on this one, but some of the comments are also true for the current example used(show mobile always due to the preview width).
If any block is styled specifically(ex capitalization) the preview in GS blocks section will not match the selected block and that's fine I think. The preview is about the global styles and what is generally applied to all blocks, but specific styles override those values.
That's also true for the opposite scenario right now. |
|
I struggle to see a short-term solution here that doesn't involve a toggle on the preview itself. At some point it might be nice to load the selected block into a focus canvas (like template parts, with drag handles for previewing desktop/mobile), but obviously that's a bigger change that needs more thought. Worth noting this wouldn't fix the issue on mobile, and we'd likely still need the toggle-able preview there regardless. |
getdave
reviewed
Jan 30, 2023
There was a problem hiding this comment.
Whilst I understand the concerns that the preview should show the default behaviour, I can't help but think this is a good fix for the problem at hand.
What major downsides are there to forcing the example to show the full preview? After all it is an example, so we can configure it however is best to be viewed.
I'd be in favour of merging:
- it solves a UX issue
- it does little damage/harm (it's an example)
- there is no alternative solution in sight or likely to be worked on
As always, happy to be told I'm wrong...
getdave
requested review from
scruffian,
MaggieCabrera,
ajlende and
draganescu
|
Tagged in some other regular Nav block contributors for additional opinions. |
|
I agree with @getdave 's assessment. The bug is that setting style options is invisible in the preview and this solves that. Other problems about better previews should be tackled elsewhere, I think. |
ntsekouras
force-pushed
the
update/navigation-block-example-with-no-overlayMenu
branch
from
743afa2 to
4deb020
Compare
getdave
approved these changes
Jan 30, 2023
Dropdown menus would be pretty easy to add to the example, but only the arrow would show as the user can't interact with the preview.
It worked ok for me: Maybe the typography styles are set on the block rather than the global styles?
I think this is more related to the implementation of the global styles preview, which has no relation to the selected block. I think it's the same situation for any block, like if I select a heading the preview in global styles can still look completely different (including different content): |
|
I'll merge this and we can create a different issue to discuss if we should change what BlockPreview shows - like reflecting the selected block. Thanks for the feedback all! |
ntsekouras
deleted the
update/navigation-block-example-with-no-overlayMenu
branch
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?
Resolves: #47130
Testing Instructions
Navigationblock preview is not showing the mobile viewThis will be effective for any preview of the block, but you can test for example in:
blockssectionBefore
After