Skip to content
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

Polish gallery inspector, inspector headings, slider #2020

Merged
merged 5 commits into from
Jul 31, 2017

Conversation

jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Jul 26, 2017

Another sort of polish kitchen sink PR. This PR changes:

  • The lavel of the "Save" button to say "Save Draft". This reads better when editing a published post, and will be further improved when autosave kicks in.
  • Inspector headings were polished a bit, and I added a "Block Settings" heading to the global CSS class option.
  • The range slider was styled to almost fix Improve range slider design #1523

With regards to the slider, it has a focus style, so it should be accessible. It suffers from that same annoying issue that any other styled elements do -- that is, the focus lingers even when you click, which it doesn't do if the component itself is unstyled. But this is discussed separtely in #904.

Slider screenshot:

screen shot 2017-07-26 at 13 13 10

@youknowriad
Copy link
Contributor

@jasmussen just a side note, I think the "Save" link actually saves the published post (and not only as a draft)

@jasmussen
Copy link
Contributor Author

Oh interesting. We should then hide the save link if the post is published. We do that on .com also.

Copy link
Contributor

@afercia afercia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems Firefox doesn't inherit box-sizing: border-box; on the slider thumb, it should be set directly on it.

@afercia
Copy link
Contributor

afercia commented Jul 26, 2017

@jasmussen Seems Firefox (tested on macOS and Windows) needs box-sizing: border-box; set directly on the slider thumb. This seems to be a bug in Firefox. See screenshot without (left), with (center), and the rounded focus style doesn't work (right). As long as there's the native thin dotted border, it's fine for a11y.

The whole control is also a bit shifted to the top, because of the negative margin.

screenshot 4

Microsoft Edge has the same issue: the thumb is too big, and the top part is also cut-off:

screenshot 5

Not sure how to fix it, but seems there are proprietary -ms- prefixed properties, see https://css-tricks.com/styling-cross-browser-compatible-range-inputs-css/

Can't test in IE 11 because of #1397 and #1863

Noticed a couple more things, will split them in separate issues.

height: 3px;
cursor: pointer;
background: $light-gray-500;
border-radius: 1.5px;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd consider to do border-radius: 50%;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did that initially, but because the track isn't a square that means the whole track becomes an oval.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right!

@jasmussen
Copy link
Contributor Author

Thanks so much for the excellent review. I will address your notes hopefully tomorrow!

@mtias mtias added General Interface Parts of the UI which don't fall neatly under other labels. Design [Feature] Inserter The main way to insert blocks using the + button in the editing interface labels Jul 26, 2017
@jasmussen
Copy link
Contributor Author

jasmussen commented Jul 27, 2017

Okay I think I have addressed the feedback.

Chrome:

screen shot 2017-07-27 at 10 23 59

Edge:

capture

The focus outline is a little smaller because apparently the range control slider on Edge crops overflow, so we have to stay within the boundaries of the control itself.

Firefox:

screen shot 2017-07-27 at 10 27 19

@melchoyce
Copy link
Contributor

Do you have a quick tl;dr for the choice to move to an outlined toggle selector instead of a filled one?

@jasmussen
Copy link
Contributor Author

Are you referring to the range "knob" or the switch?

In case it's the switch, that was an accessibility thing (on focus it's filled in). Andrea knows more.

In the case of the screenshots here, the screenshots include the focus rectangle. If you click outside the range slider, the blue outline disappears.

@melchoyce
Copy link
Contributor

The switch. Thanks for the details 👍

@afercia
Copy link
Contributor

afercia commented Jul 27, 2017

For reference, see #1988

Copy link
Contributor

@melchoyce melchoyce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still struggling with my broken Vagrant, but your screenshots look good to me. If there's any bugs I find post-merge I'll open new issues.

@jasmussen
Copy link
Contributor Author

@afercia do my corrections look good? If so I'll try and rebase and get this merged. Thanks.

@afercia
Copy link
Contributor

afercia commented Jul 31, 2017

@jasmussen looks good to me! Re: the range slider, here's some screenshots:

Chrome
chrome

Firefox
firefox

Safari
safari

Ie 11
ie11

Edge
edge

(in IE11 and Edge the "circular focus" is a bit cut off, not sure that can be fixed)

@jasmussen
Copy link
Contributor Author

Thanks!

(in IE11 and Edge the "circular focus" is a bit cut off, not sure that can be fixed)

It can be, but it means making the switch smaller. I intentionally made it big so as to be a big touch target. What do you think?

@afercia
Copy link
Contributor

afercia commented Jul 31, 2017

Ship it! 🙂 I think we can leave with a minor visual glitch in favor of better usability.

It can be confusing when editing a post, and "Update" is the actual change that makes a difference.
This gets us closer, but not fully, to fixing #1523.
Polish the range slider for IE, Chrome and Firefox. This also optimizes the CSS a little bit.
@jasmussen jasmussen force-pushed the polish/gallery-inspector-inserter branch from da18cbd to fa9f195 Compare July 31, 2017 11:22
@jasmussen
Copy link
Contributor Author

Merging after a rebase, fixed tests, and two thumbs up.

@jasmussen jasmussen merged commit 1d7b1c6 into master Jul 31, 2017
@jasmussen jasmussen deleted the polish/gallery-inspector-inserter branch July 31, 2017 11:55
@aduth
Copy link
Member

aduth commented Jul 31, 2017

Noting that with changing the label from "Save" to "Save Draft", things get a little cramped on mobile:

iPhone 6 iPhone 5 / SE
iPhone 6 Header iPhone 5 Header

Maybe the label should be contextual to the screen size? Or shuffle around some padding real estate (e.g. "Visual" looks more comfortable than others in the header).

@afercia
Copy link
Contributor

afercia commented Jul 31, 2017

I'd recommend to consider also those strings can be longer in other languages.

@nic-bertino
Copy link

I don't love this concept, but it might be helpful to group publishing-related actions together. Here's a fast wire:

publishing

@jasmussen
Copy link
Contributor Author

Great observation, Andrew and Andrea both. There isn't really room for the label, because we can't know what it might be translated to. Though I do agree, the Visual tab should have its paddings slightly condensed, at least at the mobile breakpoint.

I feel like the solution here is a) keeping our minds open for a better editor mode switch in #1671, and revisiting #967 to add an icon and/or label to unsaved states. See also #967. Let me expand that a bit, and also respond to @nic-bertino (thank you for the mockup!).

I feel strongly that "Save" is an action we can retire in the long term, and that it is our responsibility to use technology to spare the user of menial tasks. Modern web-apps auto save, and rarely have explicit save buttons. We recently added auto-save to Gutenberg, though I acknowledge this can be improved even further. As such, I strongly feel like we shouldn't optimize towards having a save button at all. As Andrea has mentioned in the past, so long as we have a save button we should definitely design for it, but I think there's a balance we need to keep.

The "Save Draft" button becomes further confusing when a post is already published. What should it do in that case, convert it to a draft? By working towards retiring the button, we can simplify the UI, as well as save the user from having to click to save their work.

More important than where to put the save button, I feel like we should keep the save state indicator where it is. This is the informational area in the top left that says Saved, Saving..., Save Failed, etc. In my research of other editors, this always came back as a super important affordance for letting you know whether your work was saved or not, especially important when relying on Autosave. But this could be an icon. Google Keep has that:

keep

Right now we have:

  • Save (when changes are detected, and autosave hasn't yet kicked in)
  • Saving...
  • [icon] Save Failed! Try again
  • [icon] Saved

We could rethink these for mobile, and ensure every state has an icon, and then show icon + label on desktop, then icon only on mobile. And indeed, perhaps we shouldn't show Save at all on mobile? It's extra anachronistic on this platform. So we could end up with:

  • [icon] Unsaved changes
  • [icon] Saving...
  • [icon] Saved

We could then show notices for failure actions instead.

Thoughts? Then I can open a separate ticket.

@nic-bertino
Copy link

I agree for the most part. One observation is that there is a reliance on iconography at the mobile view right now, which I don't think is super intuitive. I'm an experienced user and I thought the eye icon was post visibility, not preview. I think that particular UI is being stuffed with icons/actions at the moment. I'll think about it, do some research, and try to come up with some wires.

The "Save Draft" button becomes further confusing when a post is already published. What should it do in that case, convert it to a draft? By working towards retiring the button, we can simplify the UI, as well as save the user from having to click to save their work.

This can get tricky. I see a use case where someone is editing an already-published post, in which case a draft is the editing workspace until the post is re-published.

@karmatosed
Copy link
Member

Really interesting PR, what a lot of awesome discussion.

Putting my mobile hat on, the change from 'save' to 'save draft' does seem a problem. We have a lot of issues on mobile beyond this though, so worth thinking a little about our approach overall.

As @joen you say, @afercia makes an excellent point about other languages increasing the length of that string to.

I would +1 to 'save' going away. It's a little mental model shift 'did things even save' but for me an ideal 'magic' flow would be:

  • Auto saves as you go, these are drafts.
  • You make a conscious 'publish' decision which takes everything out of drafts.

I think rethinking in terms of mobile but maybe then having for desktop too makes sense. After all, if it's a better flow why not have it on desktop? Maybe there are technical reasons I'm breezing past though :)

Assuming I am not totally in the wrong place with this idea, I do like the idea of this flow for all devices, large and small:

  • Unsaved changes
  • Saving (magic just does without you clicking)
  • Saved (magic just did)
  • Publish: final action

@afercia
Copy link
Contributor

afercia commented Aug 2, 2017

We have a lot of issues on mobile beyond this though, so worth thinking a little about our approach overall.

Yep, there have been some attempts to re-think a bit the toolbar, without great success so far :) See also:
#1963 definitely a POC following the design proposal by @hedgefield nad @moorscode in #467

I still think the "Publish" button at the top is a big problem for all keyboard users, it would be great to experiment to move it after the content or, maybe, have a second Publish button at the end of the content. #riskylife

@melchoyce
Copy link
Contributor

I still think the "Publish" button at the top is a big problem for all keyboard users, it would be great to experiment to move it after the content or, maybe, have a second Publish button at the end of the content. #riskylife

Exploring a second publish button would be cool. I'd be a 👎 for only showing publish at the bottom of the screen, though, especially because of prior testing we've done on a bottom placement:

The only significant problem with the new Write screen was the bottom bar that contained the elements of the former Publish module. This is an example of how a feature that was unsuccessful in terms of performance was actually successful in terms of the testing goals. Even though participants felt the design of it was not effective, it brought out comments about the desirability of having a persistent status module so they wouldn’t have to scroll up and down to get to the Publish button. If this element had not been included in the prototype, we might not have gained this information. All users, with many comparing it to an ad banner or browser chrome, overlooked the bottom bar. Most identified the color and placement as a problem, and felt it took up too much room. Almost all requested its return to the upper right. With the addition of drag and drop expandable modules, the reduction in page scrolling would be likely to keep the Publish module relatively persistent. Even when they were used to the bottom bar, many forgot to look there when asked to preview or publish their posts, as the eye tracking data confirms.

From the Crazyhorse usability testing report.

@afercia
Copy link
Contributor

afercia commented Aug 2, 2017

@melchoyce Crazyhorse was long time ago... (in a galaxy far, far away) . I guess those testing session didn't include keyboard users, right? 🙂

@melchoyce
Copy link
Contributor

Not sure! But I think the general findings (that people don't notice actions at the bottom of their desktop screens) is still pretty legit.

@afercia
Copy link
Contributor

afercia commented Aug 2, 2017

May be. Id encourage you to test Gutenberg using only the keyboard 🙂

@melchoyce
Copy link
Contributor

I get that your issue is that it's hard for keyboard users to get back to Publish — that's totally fair! But moving the button to somewhere sighted users won't be able to find it isn't a good solution, either. We need to explore options that work for everyone.

@afercia
Copy link
Contributor

afercia commented Aug 2, 2017

We need to explore options that work for everyone

Totally agree. That doesn't seem to be the case, so far 😞

@melchoyce
Copy link
Contributor

Unfortunately I think a lot of that is ignorance on our parts, which is why your feedback has been invaluable. Do you know if any other folks from the Accessibility team would be up for contributing, with either testing, code, or design?

@afercia
Copy link
Contributor

afercia commented Aug 2, 2017

Hope some of the a11y team members will have some time to dedicate soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Inserter The main way to insert blocks using the + button in the editing interface General Interface Parts of the UI which don't fall neatly under other labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve range slider design
8 participants