Update KDS theme token #12742

AllanOXDi · 2024-10-24T12:58:36Z

Summary

…

References

Closes #12735.

Reviewer guidance

…

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

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 · 2024-10-24T13:30:17Z

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.17.3a0.dev0_git.42.g9b97919d.pex
Windows Installer (EXE)	kolibri-0.17.3a0.dev0+git.42.g9b97919d-windows-setup-unsigned.exe
Debian Package	kolibri_0.17.3a0.dev0+git.42.g9b97919d-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.17.3a0.dev0+git.42.g9b97919d.dmg
Android Package (APK)	kolibri-0.17.3a0.dev0+git.42.g9b97919d-0.1.4-debug.apk
TAR file	kolibri-0.17.3a0.dev0+git.42.g9b97919d.tar.gz
WHL file	kolibri-0.17.3a0.dev0+git.42.g9b97919d-py2.py3-none-any.whl

AlexVelezLl

Hey @AllanOXDi! For this change we need to update the KDS version from 4.4.0 (the current version installed in release-v0.17.x) to 4.6.0 which is the version that introduces the theme token updates. Between these versions there are two breaking changes that we will need to solve:

In 4.5.0: There is a breaking change that was the removal of the internal state of KCheckbox, to handle this breaking change we need to backport this commit that handled this breaking change in develop, but is not present yet in release-v0.17.x
In 4.6.0: Here we introduced the theme token updates, you can see linked the whole mapping, I see that we still need to update some more theme tokens in this pr (e.g. the grey palette).

As we havent updated the KDS version yet, we can see some regressions like this:

Before	After

AlexVelezLl · 2024-10-24T17:55:24Z

kolibri/core/assets/src/views/AttemptLogItem.vue

@@ -59,7 +59,7 @@
      v-if="attemptLog.missing_resource"
      class="coach-content-label"
      icon="warning"
-      :color="$themePalette.yellow.v_1100"
+      :color="$themePalette.yellow.v_500"


Just a note: v_1100 values map to v_600, so we will need to fix the current changes that changed v_1100 to v_500

marcellamaki · 2024-10-24T18:23:53Z

Thanks @AlexVelezLl for the helpful notes on the KDS upgrade requirements to make those required changes clearer in terms of what needs to be added to make sure the KDS version works.

@AllanOXDi I also wanted to add, in regards to this point in the acceptance criteria

any places that use the theme token numerical values in kolibri (so, anything that aligns with the updates spec in learningequality/kolibri-design-system#775) should be updated to use the correct value. So, themeTokens.primary does not need to be updated anywhere. But, if there is a place where themeTokens.v_1000 is used, that needs to be updated to themeTokens.v_500. Note: I have not proactively evaluated how many places will need to be updated. It may be a handful, it may be a lot!

"But, if there is a place where themeTokens.v_1000 is used, that needs to be updated to themeTokens.v_500" was just one example to illustrate that any situations where the numerical values is used does need to be updated, vs. themeTokens.primary which does not. This will have to be updated for all of the numbers. I didn't write this in a clear way. The mapping that Alex linked above and in that linked issue in the acceptance criteria shows all of the values that have been updated. The current state of v0.17.x is in the image on the color with the hex code, and the new number that we need to replace it with is what is in the red. For the "Brand" colors 200 becomes 100, 400 becomes 200, etc. If you have more questions about how to interpret the spec, please let me know!

…kbox KCheckbox no longer maintains its own internal state, rather, it is just like a fancy button which emits an event that it's been clicked and the parent component must react and update the value it passes to `checked` or `indeterminate` accordingly. These tests rely on KCheckbox to emit its internal state whenever it emits `change` and that no longer is the case given KCheckbox doesn't keep an internal state. I've tweaked some of the conditionals in UserTable that relied on that emitted `checked` value so that they instead get the same information from existing available values.

marcellamaki

Thanks for the work on this @AllanOXDi! Overall it looks good - I appreciate the detail-oriented work in you last updates. There are few small places where the tokens were missed or there is some other mis-match, which I've noted for reference, and I'll be opening a follow up PR momentarily so we can get those resolved and into the beta today.

Files with remaining theme tokens:

PermissionsIcon.vue
ProgressBar.vue
HeaderTab.vue
ToggleHeaderTabs.vue

marcellamaki · 2024-10-25T18:52:10Z

kolibri/core/assets/src/views/ImmersiveToolbar.vue

@@ -115,8 +115,8 @@
      },
      linkStyle() {
        const hoverBg = this.isFullscreen
-          ? this.$themeBrand.secondary.v_1100
-          : this.$themePalette.grey.v_600;
+          ? this.$themeBrand.secondary.v_300


I think here, because you previous had updated some of the code from v_1100 to v_600, you have now converted "twice", and there are a few places where you've changed v_1100 to v_300. First, from v_1100 to v_600 (correct) and then when you went through with the additional updates, you changed the already updated v_600 to v_300.

I'll mark the places where this "double" remapping has happened

marcellamaki · 2024-10-25T18:52:21Z

kolibri/core/assets/src/views/Navbar/NavbarLink.vue

@@ -63,7 +63,7 @@
        return {
          color: this.$themeTokens.text,
          ':hover': {
-            'background-color': this.$themeBrand.secondary.v_1100,
+            'background-color': this.$themeBrand.secondary.v_300,


marcellamaki · 2024-10-25T18:53:02Z

kolibri/plugins/coach/assets/src/views/common/QuizStatus.vue

@@ -341,10 +341,10 @@
      cancelStyleOverrides() {
        return {
          color: this.$themeTokens.textInverted,
-          'background-color': this.$themePalette.red.v_1100,
+          'background-color': this.$themePalette.red.v_300,


marcellamaki · 2024-10-25T18:55:36Z

kolibri/plugins/device/assets/src/views/DeprecationWarningBanner.vue

@@ -10,7 +10,7 @@
        <KIcon
          icon="warning"
          class="icon"
-          :color="$themePalette.yellow.v_1100"
+          :color="$themePalette.yellow.v_600"


The token on line 6 should also be updated

upgrades kds themetokens

ffb0a76

AlexVelezLl self-requested a review October 24, 2024 17:32

AlexVelezLl reviewed Oct 24, 2024

View reviewed changes

nucleogenesis and others added 2 commits October 25, 2024 18:30

maps V_1100 themetoken to V_600

a1de0c8

AllanOXDi force-pushed the upgrade-themetokens branch from ef7d7ef to a1de0c8 Compare October 25, 2024 15:47

updates themetoken values in all files

c274de2

github-actions bot added DEV: renderers HTML5 apps, videos, exercises, etc. APP: User Re: User app (sign-in, sign-up, user profile, etc.) labels Oct 25, 2024

[pre-commit.ci lite] apply automatic fixes

3ed2549

marcellamaki approved these changes Oct 25, 2024

View reviewed changes

marcellamaki merged commit 826a1f1 into learningequality:release-v0.17.x Oct 25, 2024
34 checks passed

This was referenced Oct 25, 2024

Final token updates #12754

Merged

Update KDS theme upgrades #12735

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update KDS theme token #12742

Update KDS theme token #12742

AllanOXDi commented Oct 24, 2024 •

edited by AlexVelezLl

Loading

github-actions bot commented Oct 24, 2024 •

edited

Loading

AlexVelezLl left a comment •

edited

Loading

AlexVelezLl Oct 24, 2024

marcellamaki commented Oct 24, 2024

marcellamaki left a comment

marcellamaki Oct 25, 2024

marcellamaki Oct 25, 2024

marcellamaki Oct 25, 2024

marcellamaki Oct 25, 2024

Update KDS theme token #12742

Update KDS theme token #12742

Conversation

AllanOXDi commented Oct 24, 2024 • edited by AlexVelezLl Loading

Summary

References

Reviewer guidance

Testing checklist

PR process

Reviewer checklist

github-actions bot commented Oct 24, 2024 • edited Loading

Build Artifacts

AlexVelezLl left a comment • edited Loading

Choose a reason for hiding this comment

AlexVelezLl Oct 24, 2024

Choose a reason for hiding this comment

marcellamaki commented Oct 24, 2024

marcellamaki left a comment

Choose a reason for hiding this comment

marcellamaki Oct 25, 2024

Choose a reason for hiding this comment

marcellamaki Oct 25, 2024

Choose a reason for hiding this comment

marcellamaki Oct 25, 2024

Choose a reason for hiding this comment

marcellamaki Oct 25, 2024

Choose a reason for hiding this comment

AllanOXDi commented Oct 24, 2024 •

edited by AlexVelezLl

Loading

github-actions bot commented Oct 24, 2024 •

edited

Loading

AlexVelezLl left a comment •

edited

Loading