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

Try: Make input styles consistent #5605

Merged
merged 3 commits into from
Apr 13, 2018
Merged

Try: Make input styles consistent #5605

merged 3 commits into from
Apr 13, 2018

Conversation

jasmussen
Copy link
Contributor

Maybe fixes #4963.

This takes a stab at unifying all basic input fields to inherit and have the same styles. That includes blocks that use input fields, and input fields in the sidebar, for example the alt text on an image block.

Blocks using the PlainText component (which is intentionally unstyled by default) can apply a regular-text style that is inherited from upstream. This is now applied to the Shortcode block, which also gets a visual enhancement.

While I believe this PR is a definite enhancement and adds much needed consistency, it should also get many eyes as it unstyles some upstream styles in order to achieve the goal. For example upstream input fields get a glowing blue border, and a drop shadow inside. This unstyles that to achieve consistency in Gutenberg.

As such, before I spend too much time on this, we should decide:

  • A) what is the ideal basic input style?
  • B) once we have that, is it okay to not inherit styles from the rest of WordPress? Should we create an upstream trac ticket to apply Gutenberg input styles to the rest of the admin?

Depending on discussions, I will amend this PR. CC: @paulwilde

Border styles were also darkened slightly, CC #5482 and @afercia.

focusstyles

@jasmussen jasmussen added [Status] In Progress Tracking issues with work in progress General Interface Parts of the UI which don't fall neatly under other labels. Needs Design Feedback Needs general design feedback. labels Mar 14, 2018
@jasmussen jasmussen self-assigned this Mar 14, 2018
@jasmussen jasmussen requested a review from a team March 14, 2018 10:30
@paulwilde
Copy link
Contributor

I'll take a closer look at this tonight most likely.

One thing I've noticed is that you are specifically targeting input, surely this would affect checkboxes/radios also?

@jasmussen
Copy link
Contributor Author

Thanks. Not urgent.

One thing I've noticed is that you are specifically targeting input, surely this would affect checkboxes/radios also?

Indeed, I forgot that. This is one of the reasons it's good to test these blanket override styles, and make sure we do it only because it's the right thing to do.

@paulwilde
Copy link
Contributor

paulwilde commented Mar 14, 2018

Core specifically targets every input type individually rather than doing a straight input. It looks ugly, but it works I guess. One alternative could be to do:

input:not([type='radio']):not([type='checkbox'])

https://github.com/WordPress/WordPress/blob/master/wp-admin/css/forms.css#L7

I'd imagine when it comes to merging this with core it might be a question of whether input styles are adjusted globally rather than Gutenberg specific.

@paulwilde
Copy link
Contributor

paulwilde commented Mar 17, 2018

I've tested the branch out and have found a few errors which the highly specific nature of just scoping the global input has introduced.

Range field:

screen shot 2018-03-17 at 15 18 18

Inline link field:

screen shot 2018-03-17 at 15 34 19

Button link field:

screen shot 2018-03-17 at 15 34 43

Checkbox fields (These are fine actually, but just for reference on how these look now):

screen shot 2018-03-17 at 15 50 43

Radio fields (These are fine too, but just to show how they look now):

screen shot 2018-03-17 at 15 49 57

Moving forward I guess scoping input to input:not([type='range']) would actually suffice from my previous comment.

It might be a browser specific thing (I'm using Safari), however I've noticed the line-height of the shortcode field and those in the sidebar seems to differ, and makes the sidebar ones seem quite squashed from the y axis. I'd recommend adding a line-height: 1.5 or so for the input, textarea to make them consistent.

Screenshot of before and after for reference:

screen shot 2018-03-17 at 15 40 49

screen shot 2018-03-17 at 15 40 03

Maybe it's personal preference, or if there's something to consider from a usability/accessibility perspective, however I think I actually preferred the more focal focus style where the input border would turn blue with a bit of box-shadow to make it stand out more. It makes it a lot more obvious where your current input focus is rather than turning the input a slighter darker shade of grey. I kind of like the style which wordpress.com uses. Screenshot for reference:

screen shot 2018-03-17 at 15 30 29

@jasmussen
Copy link
Contributor Author

Thanks for the feedback. I'll continue to refine this PR (and agree with your notes) 🌟

@jasmussen
Copy link
Contributor Author

Okay, pushed some more fixes:

screen shot 2018-03-22 at 11 08 58

screen shot 2018-03-22 at 11 08 54

screen shot 2018-03-22 at 11 08 58

These two are unchanged, though:

screen shot 2018-03-22 at 11 22 21

screen shot 2018-03-22 at 11 22 17

Gutenberg inputs can now opt into the "Gutenberg" input style by applying the class regular-text. Excerpt does this, the above two could as well. Long term we'd want to unify the styles between WordPress and Gutenberg, so anything upstream that uses regular-text gets the same style.

But pushing now for discussion and thoughts.

@karmatosed
Copy link
Member

I think this is great and a good step in consistency.

@jasmussen
Copy link
Contributor Author

A note to self to maybe also look at improving the color of placeholder text. That is, ensuring that it looks like placeholder text, but still maintaining sufficient contrast. Could mean making non-placeholder text darker.

@karmatosed karmatosed removed the Needs Design Feedback Needs general design feedback. label Apr 3, 2018
@jasmussen jasmussen force-pushed the fix/input-focus-styles branch 2 times, most recently from 7b9b7b3 to f200057 Compare April 5, 2018 15:04
@jasmussen
Copy link
Contributor Author

Squashed and rebased this. Took a look at placeholder text, but turns out we've already styled the placeholder text to be as light a gray as AAA contrast allows ($dark-gray-300 is the lightest we can use).

CC: @ryelle do you know if we can use a lighter-than-AAA for placeholder text? Asking because in an incoming PR, we show image dimensions as placeholder text when unset. For example for a 400x300px image that does not yet have explicit width and height attributes, we'd show something like (incoming ASCII art):

Width: [ 400 ]
Height: [ 300 ]

☝️ but because those values are unset, they'd be light gray and placeholder values. If we could make those a lighter gray that would make it more clear that they aren't actually set yet. But curious what the contrast requirements are for placeholder text.

@paulwilde
Copy link
Contributor

paulwilde commented Apr 5, 2018

Any reason for the class name of .regular-text? Something like .form-control (Which is what Bootstrap uses) or .input-control is more obvious with its intentions, whereas .regular-text sounds like it applies baseline paragraph text formatting.

@jasmussen
Copy link
Contributor Author

Any reason for the class name of .regular-text?

No reason, other than it's the upstream WordPress form field class as far as I can tell.

@afercia
Copy link
Contributor

afercia commented Apr 10, 2018

As long as all the input fields and textareas have a visible, properly associated <label> element OR their accessible name made apparent in some way (e.g. the post title), that's fine from an accessibility perspective.

@afercia
Copy link
Contributor

afercia commented Apr 10, 2018

@jasmussen on Safari, when I click on the "Document" or "Block" buttons (the "tabs") in the sidebar, i see the buttons text color changes to white while clicking (as far as I can tell it's the Safari default for buttons in the active state). I don't think it's related to this PR though.

@paulwilde
Copy link
Contributor

@jasmussen on Safari, when I click on the "Document" or "Block" buttons (the "tabs") in the sidebar, i see the buttons text color changes to white while clicking (as far as I can tell it's the Safari default for buttons in the active state). I don't think it's related to this PR though.

This is unrelated. I've been seeing this since day 1 pretty much. 😄

@jasmussen jasmussen force-pushed the fix/input-focus-styles branch 2 times, most recently from 6ac4fee to bd56a38 Compare April 11, 2018 12:47
@jasmussen
Copy link
Contributor Author

I've rebased, squashed, polished, fixed tests, and I believe this is ready to go.

In the last commit, ee70ff6, I also override core styles. We do this because if we don't, we can't fix #4963, because the editor itself relies so on textfields. We could replicate the same styles in the slimmer scope, but as suggested in this thread it might be worth looking at new styles that are more compatible with the general Gutenberg design, and then submit these upstream as well.

Alternately, we can revert that last commit.

Screenshots:

screen shot 2018-04-11 at 15 36 48

screen shot 2018-04-11 at 15 36 59

screen shot 2018-04-11 at 15 37 08

screen shot 2018-04-11 at 15 43 06

screen shot 2018-04-11 at 15 43 31

@paulwilde another look? Thank you!

@paulwilde
Copy link
Contributor

paulwilde commented Apr 12, 2018

@jasmussen Just tried it out, looks good.

If I had any nitpicks, it would be to change the focus outline to $blue-medium-600 (#00a0d2) as the grey against the light blue looks a little dull to me. However I don't mind too much either way.

Additionally (although it doesn't bother me as much) I think the left/right padding of 10px is a little too deep for what are somewhat small inputs text size wise. Reducing it down 1 or 2 pixels looks better and then keeps it consistent with .components-text-control__input which has them set as 8px. Also core doesn't have deep padding either (3px 5px vs 6px 10px).

Before:
screen shot 2018-04-12 at 15 16 40

After:
screen shot 2018-04-12 at 15 21 39

Maybe fixes #4963.

This takes a stab at unifying all basic input fields to inherit and have the same styles. That includes blocks that use input fields, and input fields in the sidebar, for example the alt text on an image block.

Blocks using the PlainText component (which is intentionally unstyled by default) can apply a `regular-text` style that is inherited from upstream. This is now applied to the Shortcode block, which also gets a visual enhancement.
@jasmussen
Copy link
Contributor Author

jasmussen commented Apr 13, 2018

Great thoughts, @paulwilde, thanks!

If I had any nitpicks, it would be to change the focus outline to $blue-medium-600 (#00a0d2) as the grey against the light blue looks a little dull to me. However I don't mind too much either way.

Here's $blue-medium-600:

600

Did you mean 300? Which looks like this:

300

(I used the 300 blue, let me know if you meant something else)

I took your suggestion for the padding. Good to go?

@paulwilde
Copy link
Contributor

paulwilde commented Apr 13, 2018

@jasmussen Sorry, by outline I mean the 1px border around the actual input when its in focus. It seems to be a CSS outline which is why I said outline. 😄

If you zoom in between my 2 images you should notice what I mean.

So change the focus style to be:

outline: 1px solid $blue-medium-600;
box-shadow: 0 0 0 2px $blue-medium-200;

Then I think everything looks good to go. 👍

@jasmussen
Copy link
Contributor Author

Awesome, looks good:

screen shot 2018-04-13 at 11 47 37

Thank you for the reviews, merging when the checks pass!

@jasmussen jasmussen merged commit aaa5464 into master Apr 13, 2018
@jasmussen jasmussen deleted the fix/input-focus-styles branch April 13, 2018 09:56
@jasmussen
Copy link
Contributor Author

🎉

@jasmussen jasmussen added this to the 2.7 milestone Apr 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
General Interface Parts of the UI which don't fall neatly under other labels. [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use consistent input field style
4 participants