-
Notifications
You must be signed in to change notification settings - Fork 4.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
Try polishing/simpilfying the header area #5300
Conversation
break; | ||
case 'cloud-upload': | ||
path = 'M14.8 9c.1-.3.2-.6.2-1 0-2.2-1.8-4-4-4-1.5 0-2.9.9-3.5 2.2-.3-.1-.7-.2-1-.2C5.1 6 4 7.1 4 8.5c0 .2 0 .4.1.5-1.8.3-3.1 1.7-3.1 3.5C1 14.4 2.6 16 4.5 16H8v-3H5l4.5-4.5L14 13h-3v3h3.5c1.9 0 3.5-1.6 3.5-3.5 0-1.8-1.4-3.3-3.2-3.5z'; | ||
break; | ||
case 'cloud': |
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.
These values changed because the existing cloud was moved upwards 1px to better align with text.
components/icon-button/style.scss
Outdated
@@ -9,6 +9,7 @@ | |||
position: relative; | |||
width: $icon-button-size; // show only icon on small breakpoints | |||
overflow: hidden; | |||
text-indent: 4px; |
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.
This enhances the IconButton component so when it has text, the icon indents the text, but it doesn't affect the shape of the IconButton if it has no text.
href={ link } | ||
onClick={ this.saveForPreview } | ||
target={ this.getWindowTarget() } | ||
icon="visibility" | ||
disabled={ ! isSaveable } | ||
label={ _x( 'Preview', 'imperative verb' ) } |
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.
label
is specific to IconButton
, and can now be removed.
className="editor-post-save-draft" | ||
onClick={ onSave } | ||
icon="cloud-upload" | ||
label={ __( 'Save Draft' ) } |
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.
The behavior of label
in this case will apply an aria-label
to the element. I guess this should probably have no practical effect other than to defer to this label in all cases (even in mobile context), which may be desirable I suppose.
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.
That was sort of the thinking behind adding this. I still feel like it may not be ideal to have two elements output here, one for mobile, one for desktop, but I can't think of a better way to make a good experience for both.
I'll leave the label in and we can revisit if need be.
@@ -1,3 +1,7 @@ | |||
.editor-post-save-draft { |
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.
Are these overriding styles? If so, wondering if we'd need added specificity in case the styles are applied out of the expected order.
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.
Pushed a fix to remove both this style, and the style it was (yes) overriding.
The code it overrode was this: https://github.com/WordPress/gutenberg/blob/master/components/icon-button/style.scss#L10 — the idea being that an icon button can have a label, but on mobile it should always just be the icon only. By applying a width and then removing that width past the mobile breakpoint, the labels would become visible as soon as there was screen realestate. Smarts that I thought were useful for the IconButton component.
But perhaps that was too smart. In the case of this PR, I wanted the Save label to be visible even on mobile, as the icon can't stand on its own, and the autosave feature isn't yet on par with other web-apps so you still need to press that button every once in a while.
Also, removing the width doesn't seem to have had any negative effects as far as I can tell. So I removed it.
Pushed some updates for your failing tests. |
Thank you for the review (and test fixes), I will address as soon as I can! |
I like it. I still wonder about the 'floating' publish button as you flow through, but that's something to tackle in another iteration. |
- Remove label - Remove width override
Addressed feedback and left some comments. Thanks again. |
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.
👍
Why is an upload icon used to indicate saving? That feels really weird. I'm not uploading anything. |
It's a "save to cloud" icon. I understand where your feedback is coming from, but it's inspired by other web-apps including Google Keep: In a way, you are uploading your saved changes to the server. The primary anachronistic change here is that there is a Save Draft button at all, when most modern web-apps automatically save. We do automatically save too, but because there's a fair bit of complexity in saving WordPress posts, we do need an explicit save button. Additionally, having an icon for this action allows us to show it as an icon-only design that fits well on mobile. Feel free to ping me on slack if you'd like to discuss this with higher bandwidth communication — I would love to avoid any misunderstandings if I missed anything. |
Further, when / if we have offline support, the icon / state could show whether a post is saved locally vs saved to the server origin. |
The current header area features three button types:
This PR reduces that to just two, Icon buttons and normal buttons, which lightens the whole area optically, and adds consistency.
In addition to this, it changes the "Preview" button from being an icon button into a normal, gray, button, with no icon. This is in response to usability tests, which found that the eye on its own was not obvious as "Preview".
It also adds an updated Dashicons sprite, which features a new "Save to cloud" icon, for use with the "Save Draft" icon button. Upstream PR: WordPress/dashicons#274
GIF