Display device icon on "preview" and "view" buttons #50218
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
|
Size Change: +250 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
|
I like it:
Hover state shows the metrics:
Resizing works well too:
I thought it was going to feel weird to see the desktop icon if you scaled down the viewport to mobile, but it's actually feeling fine:
We really need a better disabled state for these buttons, it's weird they suddenly get a background: That's separate, but it's worth doing. CC: @WordPress/gutenberg-design — seems like a disabled button could be the same style as the ✓ Saved button — white bg, gray text. One thing worth tinkering with, is the space between the icon and the label, which feels extra large in the mobile device version due to the metrics of the icon:
But even in the desktop state, the spacing looks a bit large:
The spacing apperas to be 8px: Just making that 4px feels good to me:
|
|
I don't think we should locally modify the spacing for this button alone, and if we do it for the mobile type only we'd get some layout shift. |
|
Flaky tests detected in 1142f8b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4851442110
|
I should've been clearer: I was suggesting we modify the default spacing here across all buttons with icons. It feels off across every variation:
4px may be too small? 6px could be better? I'm also happy to explore this separate if you prefer to keep things focused. |
|
Ah, ok :) Yes, let's do that separately |
jasmussen
approved these changes
May 2, 2023
|
I like the icons, but it feels a bit of a shame to increase the horizontal space occupied by top bar children. Do we need the 'Preview' text, couldn't that be a tooltip? |
|
@jameskoster I think that's a next step to consider, but it's a slightly larger change we should combine with the work on document title and a way to display the view link. If we have a final design target there we can move faster. |
37 tasks
8 tasks
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.
Small clarity improvement in conjunction with #50217.