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

Simplify the block selected state #19030

Open
wants to merge 2 commits into
base: master
from

Conversation

@youknowriad
Copy link
Contributor

youknowriad commented Dec 10, 2019

Related #18667

Now that there's no hover state, is there any reason to keep the large left border?
This PR removes it.

Capture d’écran 2019-12-10 à 2 06 49 PM

@youknowriad youknowriad force-pushed the update/simpler-selected-state branch from 253fc5d to d6fe7b9 Dec 10, 2019
@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor

ZebulanStanphill commented Dec 10, 2019

Adding the "Needs Accessibility Feedback" label because I vaguely recall there being color contrast reasons for having the strong left border due to the rest of the border not having high enough contrast. It would be good to make sure this change makes sense from an accessibility standpoint.

@pablohoneyhoney

This comment has been minimized.

Copy link

pablohoneyhoney commented Dec 10, 2019

We could explore a better contrast ratio for the existing border instead of an added UI to solve the former.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Dec 11, 2019

While I haven't been a fan of the left border and don't feel like it solved the problem it meant to solve, it seems like the better solution is to explore some of the focus style enhancements being discussed in #18667 and do those together. As an interim step towards that, suspect this will only confuse.

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Dec 11, 2019

Thanks all for your feedback, I pushed a commit to use a consistent and darker color for the borders inside the canvas. (I didn't touch the ones outside the canvas).

The good thing with the last commit is that now we can tweak the border color by just changing a single SASS variable.

The problem though is now we do feel inconsistency between the canvas and the rest of UI so the question again is where to draw the line? What is the minimum shippable version? Will we have to do it all at once, how can we do it step by step?

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Dec 11, 2019

Capture d’écran 2019-12-11 à 9 19 23 AM

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Dec 11, 2019

I don't think this UI change can be done step by step. This is one of those where we need to finalize the visuals we want, and implement that. This definitely removes the left border, that's good, but it's transversal to the explorations in #18667 which are still receiving thoughts and feedback.

@youknowriad youknowriad requested a review from ItsJonQ Dec 11, 2019
@karmatosed

This comment has been minimized.

Copy link
Member

karmatosed commented Dec 11, 2019

I don't think this UI change can be done step by step.

Unfortunately, I feel the same. There is however a reason to get rid of the border and 'dark line' as it is very clumsy and not a great indicator as you go into child blocks, for example, the navigation.

On select right now:

image

image

Holistically, bringing the changes in with this also feels right as one.

@shaunandrews

This comment has been minimized.

Copy link

shaunandrews commented Dec 12, 2019

image

What if we change the color of the border just on the block, and leave the others as-is? This helps bump the contrast without needing the overly aggressive thick left-border.

@mapk

This comment has been minimized.

Copy link
Contributor

mapk commented Dec 17, 2019

I second the comments from Joen. We can't implement this new UI piece-by-piece and will need to finalize the direction on this before making this change. I would rather we hold off on these changes until we have a strong unified direction forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.