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

Audit and fix theme token updates against KDS #11911

Merged
merged 53 commits into from Apr 25, 2024

Conversation

MisRob
Copy link
Member

@MisRob MisRob commented Feb 21, 2024

Summary

Updates Kolibri to KDS theming updates introduced by learningequality/kolibri-design-system#551.

Contains KDS upgrade 3.0.1 -> 4.2.0. Release notes:

References

Reviewer guidance

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

@github-actions github-actions bot added DEV: renderers HTML5 apps, videos, exercises, etc. APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) APP: Learn Re: Learn App (content, quizzes, lessons, etc.) APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) APP: User Re: User app (sign-in, sign-up, user profile, etc.) DEV: frontend SIZE: small labels Feb 21, 2024
@MisRob MisRob changed the title Replace obsolete grey scales Audit and fix theme token updates against KDS Feb 21, 2024
@MisRob MisRob added the work-in-progress Not ready for review label Feb 21, 2024
@github-actions github-actions bot added APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) SIZE: medium labels Feb 21, 2024
@MisRob MisRob force-pushed the rebrand branch 4 times, most recently from 18246aa to 1a84f66 Compare February 21, 2024 18:09
@@ -12,7 +12,7 @@
:title="title"
:removeNavIcon="isAppContext && isTouchDevice"
type="clear"
textColor="white"
textColor="black"
Copy link
Member Author

Choose a reason for hiding this comment

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

Until we add full theming support to Keen UiToolbar, we need to use either 'white' or 'black'. This will be done later on as part of KDS maintenance.

@akolson
Copy link
Member

akolson commented Feb 27, 2024

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.

@MisRob
Copy link
Member Author

MisRob commented Feb 27, 2024

Makes sense, thanks @akolson

@radinamatic
Copy link
Member

radinamatic commented Feb 28, 2024

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!

@LianaHarris360
Copy link
Member

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.

@MisRob
Copy link
Member Author

MisRob commented Feb 29, 2024

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

@MisRob MisRob force-pushed the rebrand branch 3 times, most recently from 7ab645c to cc09e57 Compare March 5, 2024 14:33
@MisRob
Copy link
Member Author

MisRob commented Mar 5, 2024

Selects styling is fixed

@rtibbles
Copy link
Member

rtibbles commented Apr 5, 2024

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

@AlexVelezLl
Copy link
Member

AlexVelezLl commented Apr 5, 2024

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.

@rtibbles
Copy link
Member

rtibbles commented Apr 5, 2024

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

@AlexVelezLl
Copy link
Member

Nvm its not from #11847. Just tested it:

image

@rtibbles
Copy link
Member

rtibbles commented Apr 5, 2024

Well, that's a relief!

@rtibbles
Copy link
Member

rtibbles commented Apr 5, 2024

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

@MisRob
Copy link
Member Author

MisRob commented Apr 8, 2024

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?

@pcenov
Copy link
Member

pcenov commented Apr 9, 2024

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

@MisRob
Copy link
Member Author

MisRob commented Apr 11, 2024

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

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

@MisRob
Copy link
Member Author

MisRob commented Apr 11, 2024

Summary of next steps on this PR:

We have our own cancel button implementation.
This fixes two cancel buttons being shown in Chrome.
Fixes contrast issues.
Fixes contrast issues.
@MisRob
Copy link
Member Author

MisRob commented Apr 15, 2024

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

@MisRob
Copy link
Member Author

MisRob commented Apr 16, 2024

All feedback should now be resolved

Copy link
Member

@pcenov pcenov left a comment

Choose a reason for hiding this comment

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

Thanks @MisRob and @LianaHarris360 - I confirm that the reported issues are fixed now, and I didn't spot any new ones!

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.

Manual QA passes, great job!!! 👏🏽 💯 :shipit:

@MisRob
Copy link
Member Author

MisRob commented Apr 17, 2024

Great work everybody :)!

Copy link
Member

@akolson akolson left a comment

Choose a reason for hiding this comment

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

Hi @MisRob! Great work on this whale PR :). The changes look good to me! Thanks.

I just left a non-blocking comment but thats about it from me!

@rtibbles rtibbles merged commit 8a8db2a into learningequality:release-v0.16.x Apr 25, 2024
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) APP: Learn Re: Learn App (content, quizzes, lessons, etc.) APP: User Re: User app (sign-in, sign-up, user profile, etc.) DEV: frontend DEV: renderers HTML5 apps, videos, exercises, etc. SIZE: large SIZE: medium SIZE: small TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants