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

Refine side UI #6141

Merged
merged 10 commits into from
Apr 18, 2018
Merged

Refine side UI #6141

merged 10 commits into from
Apr 18, 2018

Conversation

jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Apr 12, 2018

This PR optimizes and refines the side UI. In doing so, it fixes #5400 and #4223 by making the arrows visually bigger. It also simplifies the hover styles and improves mobile in the process.

These are now essentially "icon buttons", in that they have the same hover, click and border radius style. Although the buttons are only slightly bigger, the visually bigger hit area makes it feel more clickable as well.

refine side ui

As you can see, I also snuck in a trash button. Let me know your thoughts on that — it's easy to remove and restore later on, if we need to do that to ship the other enhancements, but it's very nice to have it surfaced there, and brings with it a nice consistency with mobile.

@jasmussen jasmussen added [Type] Enhancement A suggestion for improvement. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). labels Apr 12, 2018
@jasmussen jasmussen self-assigned this Apr 12, 2018
@jasmussen jasmussen requested a review from a team April 12, 2018 10:52
@chrisvanpatten
Copy link
Member

chrisvanpatten commented Apr 12, 2018

This looks like a huge improvement although the rounded border radius bumping right up against the block border feels a bit off.

refine_side_ui_by_jasmussen_ pull_request__6141 _wordpress_gutenberg

I couldn't figure out a way to do it perfectly in code (you'd probably need to apply the border styles to the SVG, or a new nested child element? are there any examples in Gutenberg doing something similar?) but I mocked up how it might look:

untitled___ paleo_meal_plans _wordpress

That extra bit of breathing space feels good, and if it's treated as clickable area on the button it should keep it easy to hit.

(Edit: would obviously want to do the same on the right side!)

/>
);
/>,
<BlockRemoveButton key="remove" uids={ uids } role="menuitem" />
Copy link
Member

Choose a reason for hiding this comment

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

I think the role should be removed here.

@ZebulanStanphill
Copy link
Member

ZebulanStanphill commented Apr 12, 2018

Personally, I am glad to see the trash button return. It is pretty handy, and I do not think it adds much weight to the UI, especially with the changes in #6101.

These new styles look pretty nice and do feel more clickable, which is good. I like @chrisvanpatten's suggestion of having a little gap between them and the visible block outline.

@noisysocks
Copy link
Member

noisysocks commented Apr 13, 2018

Love it! Do we need to have both the trash icon and the Remove option in the menu?

Joen Asmussen and others added 6 commits April 13, 2018 10:53
@gziolo
Copy link
Member

gziolo commented Apr 13, 2018

Great improvements, I have a similar concern regarding this very subtle thing with overlapping borders:

screen shot 2018-04-13 at 11 06 35

@gziolo gziolo added this to the 2.7 milestone Apr 13, 2018
@jasmussen
Copy link
Contributor Author

Fixed the flickering issue that made things look slightly cropped in weird circumstances, and added a little clearance as suggested by @chrisvanpatten:

side ui

Thanks for the reviews folks, good to go?

@gziolo
Copy link
Member

gziolo commented Apr 13, 2018

I noticed regression on mobile caused by the fact that icons are swapped on the right side.

Before

screen shot 2018-04-13 at 14 41 47

screen shot 2018-04-13 at 14 42 00

After

screen shot 2018-04-13 at 14 38 06

screen shot 2018-04-13 at 14 38 28

The issues with z-index is unrelated to this PR, but nice to fix anyways :)

Otherwise everything looks great and is ready to go. We just need to make sure there is no regression introduced.

@jasmussen
Copy link
Contributor Author

I pushed a fix for the z-index, however I would appreciate some help with the dropdown menu. Turns out there are issues on all the breakpoints due to the addition of the BlockRemove button:

screen shot 2018-04-16 at 08 45 52

screen shot 2018-04-16 at 08 46 26

screen shot 2018-04-16 at 08 46 35

@gziolo if you have bandwidth to look at it, I'd really appreciate it, thank you.

@jasmussen
Copy link
Contributor Author

Thanks so much Riad! Okay now, thanks to Riad, this is ready for final review.

@jorgefilipecosta
Copy link
Member

jorgefilipecosta commented Apr 17, 2018

It looks like we have a small white line at the side of the mover (noticed when the background is not white):
image.

padding: ( $block-side-ui-width - 18px ) / 2; // this makes the SVG fill the whole available area, without scaling the artwork
}

// Apply a background in nested contexts, only on desktop
Copy link
Member

@jorgefilipecosta jorgefilipecosta Apr 17, 2018

Choose a reason for hiding this comment

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

In nested contexts on mobile the contrast may be low:
image
But the result is better than what we had and finding a solution for this cases is really hard as the background color of the nested context is impossible to predict.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mix blend mode could help with that. Not the greatest support though.

@jorgefilipecosta
Copy link
Member

Changes here improve the look and feel in nested contexts and the PR #5452 will benefit a lot from this changes 👍 Thank you @jasmussen!

@jasmussen
Copy link
Contributor Author

Thanks for the review, @jorgefilipecosta!

Your feedback on other colored backgrounds is duly noted, and applies not only to nested contexts, but presumably also to when themes load their own stylesheets into the editor. It is on my todo list to tackle this next — perhaps using mix-blend-mode as @lsl suggested, or just replaing the grays with blacks and whites with varying opacities to better fit in, while maintaining contrast.

Do you think, perhaps, that we should merge this PR in as is, and then tackle this separately, though? It seems like it would be nice to get these changes into 2.7 if we can.

@jasmussen
Copy link
Contributor Author

Got a thumbs up in chat. Going to merge this as is, and then tackle UI contrast issues separately.

@jasmussen jasmussen merged commit e50e200 into master Apr 18, 2018
@jasmussen jasmussen deleted the update/refine-movers branch April 18, 2018 08:09
nuzzio pushed a commit to nuzzio/gutenberg that referenced this pull request Apr 25, 2018
* Polish side UI, make arrows bigger.

This is initial work to improve the side UI. It fixes WordPress#5400 and WordPress#4223 by making the arrows visually bigger. It also simplifies the hover styles and improves mobile in the process.

* Refine mobile block UI, and responsive hover labels.

* Add trash button to right side UI.

More to come.

* Enhance situation in wide blocks.

* Remove role from Remove button, and remove Trash from menu.

* Editor: Fix Eslint issues in BlockSettingsMenu

* Refine collapsing margins hack, fix flickering.

* Add some clearance between side UI and block.

* Fix z-index issue.

* Fix block settings dropdown menu
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refine the up/down arrows to balance visibility with usability better
9 participants