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

Material background on KTextbox #139

Merged

Conversation

nucleogenesis
Copy link
Member

@nucleogenesis nucleogenesis commented Dec 2, 2020

This is a draft for now - this should not be merged until KSelect is also available in KDS and has the same or similar styles applied.

Fixes #124
Fixes #167

  • Gray #e9e9e9 background on KTextbox
  • CSS changes to align things roughly like the sample field https://material.io/components/text-fields
  • Changed the value given to the translate() CSS function to ensure the label is centered (original value pushed the label to the baseline)
  • Add a   empty div fallback for the part of the DOM flow where the error/help message would show. A counter shows on the same horizontal line but is position: absolute; so when there is no help/error text, subsequent elements are closer to the previous one than they ought to be and they look squished.

I took this first screenshot w/ a delay feature on my screenshot extension so I could properly focus the field to show that the theme colors work still.

screenshot-localhost_8000-2020 12 02-13_55_14

These are just a few more examples with error messages, text, no text.+

screenshot-localhost_8000-2020 12 02-13_50_31
screenshot-localhost_8000-2020 12 02-13_52_40
screenshot-localhost_8000-2020 12 02-13_52_59

@nucleogenesis nucleogenesis added the category: library Shared code library label Dec 2, 2020
@nucleogenesis nucleogenesis added this to the v0.2.2 milestone Dec 2, 2020
@nucleogenesis nucleogenesis marked this pull request as draft December 2, 2020 22:45
@marcellamaki marcellamaki modified the milestones: v0.2.2, Q1 Feb 3, 2021
@nucleogenesis
Copy link
Member Author

I don't recall why this is a draft "Until we get KSelect" in. I actually am okay now with merging it. The fact that KSelect will still be without the background (like in the image below) isn't super offensive compared to the images included at the top.

screenshot-localhost_8000-2021 03 03-13_56_03

@indirectlylit @jtamiace do you agree?

@jtamiace
Copy link
Member

jtamiace commented Mar 3, 2021

I'm interpreting that the main benefit of waiting would be to have complete consistency once this background color change is merged in?

I guess my question is how prevalent KSelect is. If like half of them would end up with different backgrounds, may be worth waiting. If there are only a few KSelects, probably ok?

@nucleogenesis
Copy link
Member Author

@jtamiace I don't think it's very prevalent. The only places I can think of are in table filters and a couple of device and/or facility settings.

@jtamiace
Copy link
Member

jtamiace commented Mar 3, 2021

Cool. It's a 👍 for me then!

@indirectlylit
Copy link
Contributor

same here

@radinamatic
Copy link
Member

Can't but notice that if we implement the text field background, contrast becomes insufficient for labels and error message placeholders.

Label Error
2021-03-04_19-24-41 2021-03-04_19-31-29

@indirectlylit
Copy link
Contributor

indirectlylit commented Mar 4, 2021

@radinamatic can you give us the darkest compliant grey value for the label case?

The error text looks much lighter than our token which should be red 700 (#d32f2f):

image

@radinamatic
Copy link
Member

With the red 700 (#d32f2f) foreground color, darkest compliant grey value for the background would be #F5F5F5, which is already the main background color.

2021-03-04_20-26-45

Our main purple was already on the edge, so the only compliant background color would be #FAFAFA, just a notch off white 😕

fafafa

(Crazy idea)
How about we make the text fields background color full white...? 😈

@indirectlylit
Copy link
Contributor

How about we make the text fields background color full white...? 😈

@radinamatic for the context of this change see #167

@jtamiace
Copy link
Member

jtamiace commented Mar 4, 2021

We could also choose a darker purple from our palette that's compliant with #F5F5F5?

@nucleogenesis nucleogenesis force-pushed the material-styles-on-forms branch from b0040f9 to 8bab49e Compare March 18, 2021 16:06
@nucleogenesis
Copy link
Member Author

@radinamatic @jtamiace @indirectlylit I've updated to use more accessible colors - but I did not change the background to a lighter grey for reasons to be mentioned below:

WebAIM Color After (Before can be seen above or at https://kolibribeta.learningequality.org)
errortoken Primary Dark image
screenshot-webaim org-2021 03 18-09_25_30 Error token-error

So - I darkened the purple to primaryDark which is improved - but using #f5f5f5 can have product implications because that's the color in Kolibri we use for the background of the sign in page - so you don't actually see the background that we're trying to implement.

An alternative thought is that we could change the themeTokens.error to use the slightly darker red that themeTokens.incorrect uses, which passes the WebAIM AA test (but not the AAA):

screenshot-webaim org-2021 03 18-09_25_44

@nucleogenesis nucleogenesis marked this pull request as ready for review March 18, 2021 16:47
@nucleogenesis nucleogenesis requested review from jtamiace and radinamatic and removed request for indirectlylit March 18, 2021 16:47
Copy link
Member

@jtamiace jtamiace left a comment

Choose a reason for hiding this comment

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

Thanks for doing this exploration @nucleogenesis!

@indirectlylit
Copy link
Contributor

we could change the themeTokens.error to use the slightly darker red that themeTokens.incorrect uses

I chose these tokens somewhat arbitrarily – there was not an intentional design decision to use different red shades for the two meanings.

where `primary` was used

also changes `themeTokens.error` to use the `red.v_800`
that `themeTokens.incorrect` does to improve a11y and
because it wasn't chosen with any particular purpose
learningequality#139 (comment)
@nucleogenesis
Copy link
Member Author

Thanks @indirectlylit - I've changed the theme to use the darker red now - will await an approval from @radinamatic and if she's good with this then will merge

Copy link
Member

@radinamatic radinamatic left a comment

Choose a reason for hiding this comment

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

primaryDark is looking much better!!! 💯

Thank you, Jacob! 👍🏽

@nucleogenesis nucleogenesis merged commit 2cbd7ee into learningequality:v0.2.x Mar 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: library Shared code library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

upgrade text boxes to new Material style KTextbox needs box styling instead of underline
5 participants