kolibri icon indicating copy to clipboard operation
kolibri copied to clipboard

Audit and fix theme token updates against KDS

Open MisRob opened this issue 1 year ago • 5 comments

Summary

Updates Kolibri to KDS theming updates introduced by https://github.com/learningequality/kolibri-design-system/pull/551.

References

  • Closes https://github.com/learningequality/kolibri/issues/11867
  • KDS PR: https://github.com/learningequality/kolibri-design-system/pull/551
  • Designs: https://www.figma.com/file/p7aOP2MBv0SED1uhaPwqEd/Product-rebrand?type=design&node-id=426-7545&mode=design

Reviewer guidance

  • Please study the new theme first https://deploy-preview-551--kolibri-design-system.netlify.app/colors/#

Development

  • [ ] Are code changes fine?
  • [ ] Read the upgrade process below and preview regexes I used. Can you think of any important area I left out?

QA

  • [ ] Can you see some glitches in regards to colors?
  • [ ] Are contrast ratios good?
  • [ ] Please test states that are often not obviously visible - errors, warning, and similar
  • [ ] Mobile device

You may get a good sense on what to look from the "Upgrade process" notes below.

Upgrade process

Available colors and tokens can be found at https://deploy-preview-551--kolibri-design-system.netlify.app/

(1) Breaking KDS API changes

(a) KDS removed palette colors: purple, deeppurple, indigo, blue, brown, cyan, teal, lightgreen, lime, yellow, amber, deeporange, bluegrey

Search for

palette.purple|palette.deeppurple|palette.indigo|palette.brown|palette.cyan|palette.teal|palette.lightgreen|palette.lime|palette.amber|palette.deeporange|palette.bluegrey

and

palette\(\).purple|palette\(\).deeppurple|palette\(\).indigo|palette\(\).brown|palette\(\).cyan|palette\(\).teal|palette\(\).lightgreen|palette\(\).lime|palette\(\).amber|palette\(\).deeporange|palette\(\).bluegrey

and replace with closest available palette colors

(b) KDS removed grey scale values: palette.grey.v_300, palette.grey.v_500, palette.grey.v_700, palette.grey.v_900

Search for

palette.grey.v_300|palette.grey.v_500|palette.grey.v_700|palette.grey.v_900

and

palette\(\).grey.v_300|palette\(\).grey.v_500|palette\(\).grey.v_700|palette\(\).grey.v_900

and replace with the closest darker grey scale values that are available

(c) KDS removed scale values for brand and palette colors (except palette.grey): v_50,v_100, v_300, v_500, v_700, v_900

Search for

(?<!grey)\.v_(50|100|300|500|700|900)\b

and replace with the closest scale value that is available

(d) KDS removed content-related tokens: exercise, video, audio, document, html5, slideshow

Search for

tokens.exercise|tokens.video|tokens.audio|tokens.document|tokens.html5

and

tokens\(\).exercise|tokens\(\).video|tokens\(\).audio|tokens\(\).document|tokens\(\).html5

and replace with relevant tokens

(e) KDS removed other tokens: appBarFullscreen, appBarFullscreenDark, linkDark

Search for

tokens.appBarFullscreen|tokens.appBarFullscreenDark|tokens.linkDark

and

tokens\(\).appBarFullscreen|tokens\(\).appBarFullscreenDark|tokens\(\).linkDark

and replace with relevant tokens

(2) Hardcoded color values

Search the most commonly used CSS properties that can receive color

color|background|border|outline-color|box-shadow|text-shadow|fill|stroke|outline|text-decoration-color

and locate any hex, rgb(a), hsl(a),named color values to be replaced with relevant colors (themed ideally, but if blocked replace with new hardcoded color and open follow-up)

(3) Colors

Some colors in the new palette have the same name but look differently. Due to reduction in scales, they will now often look lighter. Preview colors and see if higher scale value is needed to ensure good contrast ratio or if another colors need to be used to work in the new theme. This applies particularly to non-grey colors.

(?<!grey)\.v_(200|400|600|800|1000|1100)\b

(4) Pages background color

Previously, we used palette.grey.v_100 to set page background color. It needs to be updated to lighter .v_50. There are more components where it's being set. Search for places that set some kind of background color with palette.grey.v_100 and see if it needs to be changed to palette.grey.v_50.

(5) Contrast

Check places that utilize tokens.text and tokens.textInverted together with setting background color via palette for sufficient contrast.

(6) Brand colors

Check places where brand.primary and brand.secondary is used and see if they need to be switched.


Testing checklist

  • [ ] Contributor has fully tested the PR manually
  • [ ] If there are any front-end changes, before/after screenshots are included
  • [ ] Critical user journeys are covered by Gherkin stories
  • [ ] Critical and brittle code paths are covered by unit tests

PR process

  • [ ] PR has the correct target branch and milestone
  • [ ] PR has 'needs review' or 'work-in-progress' label
  • [ ] If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • [ ] If this is an important user-facing change, PR or related issue has a 'changelog' label
  • [ ] If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

MisRob avatar Feb 21 '24 08:02 MisRob

HI @MisRob! I update the regex (?<!grey)\.v_(50|100|300|500|700|900) to (?<!grey)\.v_(50|100|300|500|700|900)\b as it was picking up any v_* including the mentioned values and not the actual values. As such, I was able to pickup v_1000. I also updated a another similar to this.

akolson avatar Feb 27 '24 08:02 akolson

Makes sense, thanks @akolson

MisRob avatar Feb 27 '24 12:02 MisRob

After the discussion about the text input fields on Slack, and subsequent changes by @LianaHarris360, that component now seems to have sufficient contrast, but there is a similar contrast issue with KSelect in the Library page:

KSelect

@LianaHarris360 again dug into code and found out that KSelect component in the file kolibri/plugins/learn/assets/src/views/SearchFiltersPanel/SelectGroup.vue has the background manually set to $themePalette.grey.v_200 which is the HEX #CCCCCC, and that is way too dark for the labels to have sufficient contrast.

Can we make sure to apply the same front- and background color combinations as for the input text fields? Thank you!

radinamatic avatar Feb 28 '24 01:02 radinamatic

I also want to flag that the original KSelect component has no background color at all, but after taking another look at the Figma specs, it should have the same background color as the other input components. I will update the KDS PR to reflect this change.

LianaHarris360 avatar Feb 28 '24 14:02 LianaHarris360

Alright, thank you. I will remove select background color override from Kolibri.

MisRob avatar Feb 29 '24 10:02 MisRob

Selects styling is fixed

MisRob avatar Mar 05 '24 15:03 MisRob

Since there's still time, I will do some more work here fixing issues that @jtamiace reported https://www.notion.so/learningequality/Kolibri-rebranded-first-preview-a8c0dc1a63a24071931dac150af0c014

MisRob avatar Mar 05 '24 15:03 MisRob

We still need to replace the logo, but other than that, I think everything that has come up so far is resolved - let's review and QA so there's time to incorporate new feedback.

MisRob avatar Apr 02 '24 12:04 MisRob

Hi @MisRob I have identified a number of issues mainly about the behavior and presentation of the modals, the search fields and the drop-down lists. As I have too many screenshots and videos I have added them to this shared folder so that you can take a look and identify the ones that are caused by changes made by this PR.

pcenov avatar Apr 05 '24 15:04 pcenov

Also, it appears that the LanguageSwitcherList component is not being displayed on the login page.

LianaHarris360 avatar Apr 05 '24 18:04 LianaHarris360

Hrm, is it possible that we'll need to backport @AlexVelezLl's work from develop around this? There's this PR: https://github.com/learningequality/kolibri/pull/11977 but I also think some previous work as well.

rtibbles avatar Apr 05 '24 18:04 rtibbles

Hmm Misha suggested not using the KListWithOverflow component in 0.16 because of IE support @rtibbles.

This seems to be a regression introduced in #11847, but I think we can just revert it if we can live with #7997 in 0.16.

And leave the more general solution from #11977 in develop.

AlexVelezLl avatar Apr 05 '24 19:04 AlexVelezLl

Does that mean that this issue is currently extant in 0.16 then?

rtibbles avatar Apr 05 '24 19:04 rtibbles

Nvm its not from #11847. Just tested it:

image

AlexVelezLl avatar Apr 05 '24 19:04 AlexVelezLl

Well, that's a relief!

rtibbles avatar Apr 05 '24 19:04 rtibbles

Seems like it is a regression from this PR specifically then.

rtibbles avatar Apr 05 '24 19:04 rtibbles

Thank you @LianaHarris360 and @pcenov for feedback, that's really valuable. I believe that after it's all resolved, the PR will be robust. @pcenov I think some of the issues will definitely be related to rebrand and some probably not. There's still time so I will see if I can work through it all here since it may be faster than opening so many follow-ups. In case there are some left, I will take care of opening issues. Would you mind if I added 'resolved' subfolder to your folder and moved resolved issues in there?

MisRob avatar Apr 08 '24 15:04 MisRob

Hi @MisRob - please feel free to create a 'resolved' subfolder or any other folders as needed. :)

pcenov avatar Apr 09 '24 10:04 pcenov

Also, it appears that the LanguageSwitcherList component is not being displayed on the login page.

@AlexVelezLl fixed it in https://github.com/learningequality/kolibri-design-system/pull/612 so after I release the new KDS version next week, it will be resolved.

MisRob avatar Apr 11 '24 10:04 MisRob

Summary of next steps on this PR:

  • [x] @MisRob to release and install the new KDS with the fix for the language switcher list not being displayed on the login page, and select and modal issues
  • [x] @MisRob to resolve https://drive.google.com/drive/u/1/folders/1C9jqrXs0h3ZYVCEpwyq5I7NAMdbBkEtO
  • [x] @LianaHarris360 to resolve https://drive.google.com/drive/u/1/folders/13n0OWwzRR62eSI87Brtl2yQr_51Q2-jw
  • [ ] @rtibbles to introduce the new logo

MisRob avatar Apr 11 '24 11:04 MisRob

@LianaHarris360 fixed the modal and select bugs found by @pcenov here https://github.com/learningequality/kolibri-design-system/pull/616 and it will be available after I create a new release and install it here.

MisRob avatar Apr 15 '24 03:04 MisRob

All feedback should now be resolved

MisRob avatar Apr 16 '24 19:04 MisRob

Great work everybody :)!

MisRob avatar Apr 17 '24 15:04 MisRob