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

Make input placeholder appear lighter #207

Merged
merged 3 commits into from
Jul 14, 2020
Merged

Conversation

thet
Copy link
Member

@thet thet commented Jul 9, 2020

The current placeholder styles make them look like normal input:
Screenshot from 2020-07-09 11-38-21

This adds an opacity of 0.5 to them to distinguish them better from normal input:
Screenshot from 2020-07-09 11-52-56

This uses @plone-gray-lighter color instead of opacity (current version of this PR):
Screenshot from 2020-07-09 12-09-43

@polyester please comment if you see any accessibility issues here.

@agitator after discussion of the implications we should forward port that to the master branch.

Setting input darker, keeping placeholder as-is:
Screenshot from 2020-07-09 14-44-27

@thet thet changed the base branch from master to 2.x July 9, 2020 09:50
@thet thet force-pushed the thet-placeholder--2.x branch from 624de7e to 0277e0f Compare July 9, 2020 09:51
@thet thet requested review from polyester and agitator July 9, 2020 10:13
@vincentfretin
Copy link
Member

I almost can't read the text with @plone-gray-lighter on my screen.

@vincentfretin
Copy link
Member

I see that plone-gray-lighter is used mainly for background and border. Please don't use it for text, this is too light.

@thet
Copy link
Member Author

thet commented Jul 9, 2020

@vincentfretin I'd use the opacity version. May even set it to 0.6 instead 0.5.

@polyester
Copy link
Member

Placeholder text must still satisfy a11y contrast rules, which this doesn't.
Generally, that would mean #767676 is the lightest you can go, for small text.
One could make the text bigger and say it's "large text", where the contrast needed is 3:1, and the lightest you can go is #959595, but the general assumption is that "large text" should be 20px minimum.

@vincentfretin
Copy link
Member

@vincentfretin
Copy link
Member

You're right #767676 is the minimum.

@vincentfretin
Copy link
Member

The text in a document is using #4d4d4d (body { color }). Some paragraph is using #696969 (.documentDescription { color })
We could define the input color as #4d4d4d so it will be darker than the current placeholder color.

@vincentfretin
Copy link
Member

vincentfretin commented Jul 9, 2020

FYI, I almost don't see a different on my desktop screen between
@plone-gray: lighten(#000, 41%); #696969
@plone-gray-light: lighten(#000, 46.5%); #767676 (@plone-input-color-placeholder is currently on this one)

If you use
@plone-input-color: @plone-gray-dark; (instead of current @plone-gray)
it's fine for me.

@plone-gray-dark: lighten(#000, 30%); is #4d4d4d

and this matches the current color for:
@plone-legend-color: @plone-gray-dark;
@plone-text-color: @plone-gray-dark;

@@ -181,7 +181,7 @@
//** Border color for inputs on focus
@plone-input-border-focus: @plone-portlet-list-bullet;
//** Placeholder text color
@plone-input-color-placeholder: @plone-gray-light;
@plone-input-color-placeholder: @plone-gray-lighter;
Copy link
Member

Choose a reason for hiding this comment

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

I would revert this back to @plone-gray-light
and use @plone-input-color: @plone-gray-dark; above.

@thet thet requested a review from vincentfretin July 9, 2020 12:45
@thet thet force-pushed the thet-placeholder--2.x branch 2 times, most recently from 16cdf2d to 797861e Compare July 9, 2020 12:48
@vincentfretin
Copy link
Member

Can you update the comment line 6 for @plone-gray-dark it says 4c4c4c but it actually generates 4d4d4d?
Can you please use this command to generate the bundle and sourcemap (I added them in the README a few days ago, I didn't know about the grunt command):

cd plonetheme.barceloneta/plonetheme/barceloneta/theme/less
npx lessc --source-map=barceloneta-compiled.css.map barceloneta.plone.local.less barceloneta-compiled.css

The grunt command generate wrong urls in the sourcemap if you over the css rule in chrome inspector.

@thet thet force-pushed the thet-placeholder--2.x branch from 797861e to 44c0b30 Compare July 10, 2020 08:14
@thet
Copy link
Member Author

thet commented Jul 10, 2020

Awesome finding!
I fixed the grunt task also - it's documented in https://github.com/plone/plonetheme.barceloneta/blob/2.x/HOWTO_DEVELOP.rst and for 2.x the development workflow should stay the same.
In master/barceloneta-lts grunt isn't used anymore.

@thet thet force-pushed the thet-placeholder--2.x branch from 44c0b30 to 8c4b4ac Compare July 10, 2020 08:17
@vincentfretin
Copy link
Member

Cool you fixed the grunt task as well.
😢 I missed completely this HOWTO_DEVELOP.rst file, I didn't see it. I'm looking first for README and CONTRIBUTING.
Why don't we put the content of HOWTO_DEVELOP.rst in CONTRIBUTING.rst? CONTRIBUTING.rst contains only a link.

@vincentfretin
Copy link
Member

Your sourcemap has indeed the correct path now. And I see it includes the source code. This is apparently not necessary because we can access the source less files already. With npx lessc the source are not included.

@vincentfretin
Copy link
Member

Otherwise the changes looks good. You can merge.

@vincentfretin
Copy link
Member

Ok I see that the file CONTRIBUTING.rst is the same in all packages, so we probably don't want to put additional stuff there if next time we update this file in all repositories.
I added a link to HOWTO_DEVELOP.rst from the README in #208

@vincentfretin vincentfretin deleted the thet-placeholder--2.x branch July 14, 2020 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants