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

Better contrast for input fields #417

Merged
merged 3 commits into from
Jan 9, 2019

Conversation

dfeyer
Copy link
Contributor

@dfeyer dfeyer commented Jan 8, 2019

This change set a white background for input field (text, select and textarea)
to have a better contrast with the page background and improve accessibility for
people with vision problem, and just crappy screen.

This change also remove the round corner for the input to have a clear difference
between editing field and buttons.

This max-width of the form element are a bit bigger too.

This change set a white background for input field (text, select and textarea)
to have a better contrast with the page background and improve accessibility for
people with vision problem, and just crappy screen.

This change also remove the round corner for the input to have a clear difference
between editing field and buttons.

This max-width of the form element are a bit bigger too.
@dfeyer
Copy link
Contributor Author

dfeyer commented Jan 8, 2019

Screenshot are better than a long description

image

@@ -104,6 +100,7 @@ input[type="submit"] { display: block; }

// Writing page
form.new-post {
max-width: 60em;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will enforce the max-width for all element inside so we don't have to define the max-width for every element that can be used in a form element

@@ -47,18 +47,19 @@ small {

/// Main
body > main > *, .h-feed > * {
padding: 0 20%;
margin: 0 20%;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needed to set the max-width on form element, should not break anything

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 it broke the article list on the profile page, but that's not a real problem. However if you want to fix it, just replace padding with margin in .cards too.

Copy link
Member

Choose a reason for hiding this comment

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

(and actually it is not broken, just different from what we currently have)

@@ -119,7 +116,6 @@ form.new-post {
min-height: 20em;
overflow-y: hidden;
resize: none;
box-sizing: content-box;
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 don't have the full history of the project so I don't know why you use a different box sizing here ?

Copy link
Member

Choose a reason for hiding this comment

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

I don't remember either 😅

@dfeyer
Copy link
Contributor Author

dfeyer commented Jan 9, 2019

I forgot to say that the margin between to label and the input are smaller with this change, to make the block (label+input/textarea) more "solid".

An other issue we have here is the too low color contrast between the help text/button and the background, will try to solve this in an other PR

@dfeyer dfeyer changed the title Better contrat for input fields Better contrast for input fields Jan 9, 2019
@elegaanz elegaanz self-requested a review January 9, 2019 15:45
Copy link
Member

@elegaanz elegaanz left a comment

Choose a reason for hiding this comment

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

Thanks a lot! It indeed greatly improves contrast. My only concern is that buttons seem a bit inconsistent with the rest of the design now, since they are the only elements with slightly rounded corners (everything else has sharp angles, expecting avatars that are totally rounded). Otherwise it looks great! Thanks again.

@@ -47,18 +47,19 @@ small {

/// Main
body > main > *, .h-feed > * {
padding: 0 20%;
margin: 0 20%;
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 it broke the article list on the profile page, but that's not a real problem. However if you want to fix it, just replace padding with margin in .cards too.

@@ -47,18 +47,19 @@ small {

/// Main
body > main > *, .h-feed > * {
padding: 0 20%;
margin: 0 20%;
Copy link
Member

Choose a reason for hiding this comment

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

(and actually it is not broken, just different from what we currently have)

@@ -119,7 +116,6 @@ form.new-post {
min-height: 20em;
overflow-y: hidden;
resize: none;
box-sizing: content-box;
Copy link
Member

Choose a reason for hiding this comment

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

I don't remember either 😅

@dfeyer
Copy link
Contributor Author

dfeyer commented Jan 9, 2019

My only concern is that buttons seem a bit inconsistent

@BaptisteGelez Button are my next topic, will need a better visual language to make them different from input, but not too much. Maybe playing with the height of the element, and use plume violet color for the primary action as a background color. Will push an other PR when my design research are more advanced, will try to prototype on figma and share the design here before the implementation.

@elegaanz
Copy link
Member

elegaanz commented Jan 9, 2019

Okay! Then unless you want to fix this little "issue": #417 (comment) I will merge this PR (but I'm totally fine merging it as it is if you don't want to take the time to do the change, I completely understand).

@dfeyer
Copy link
Contributor Author

dfeyer commented Jan 9, 2019

Let's merge this one, and I can fix the margin of cards in an other PR, I hate swapping padding / margin all the time, so maybe we can found a cleaner solution.

@codecov
Copy link

codecov bot commented Jan 9, 2019

Codecov Report

Merging #417 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #417   +/-   ##
=======================================
  Coverage   27.64%   27.64%           
=======================================
  Files          63       63           
  Lines        7336     7336           
=======================================
  Hits         2028     2028           
  Misses       5308     5308

@elegaanz elegaanz merged commit 09a26b4 into Plume-org:master Jan 9, 2019
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.

2 participants