Removed theme-dependent SCSS variables #886

shruti862 · 2025-01-12T09:54:12Z

Description

This PR removes theme-dependent SCSS variables in the file lib/keen/styles/variables.scss and replace them with the corresponding JS/Vue theme variables.

Issue addressed

Addresses issue #617

Addresses #PR# HERE

Before/after screenshots

Changelog

Description: Removed SCSS variables in lib/keen/styles/variables.scss that relied on theming SCSS variables. Replaced their usage in KDS components and styles with direct references to the corresponding JS/Vue theme variables using inline style bindings or dynamic classes.
Products impact: none
Addresses: Issue Remove theme-dependent SCSS variables: Remove variables within KDS files that rely on theming SCSS variables #617
Components: No public KDS components directly affected.
Breaking: no
Impacts a11y: no
Guidance: Consumers of KDS should ensure they are not relying on the removed SCSS variables. If theming is applied, they must directly reference the appropriate KDS color variables in Vue templates for consistency and runtime theming support.

Steps to test

Step 1
Step 2
...

(optional) Implementation notes

At a high level, how did you implement this?

Does this introduce any tech-debt items?

Testing checklist

Contributor has fully tested the PR manually
If there are any front-end changes, before/after screenshots are included
Critical and brittle code paths are covered by unit tests
The change is described in the changelog section above

Reviewer guidance

Is the code clean and well-commented?
Are there tests for this change?
Are all UI components LTR and RTL compliant (if applicable)?
Add other things to check for here

Comments

LianaHarris360

Thank you for your quick work on this, I apologize for the delayed review. I noticed that some of the variables that were taken out of variables.scss are still being used in other places. Could you please check again to make sure that all of the removed styling variables aren’t being used elsewhere?

Also, could you please fill in the Changelog descriptions in your PR template? If you need any help with this, just let us know!

lib/keen/styles/variables.scss

shruti862 · 2025-01-22T17:59:29Z

Hey @LianaHarris360 ,Thank you for letting me know ! . I will surely revisit the variables that were removed from variables.scss to ensure they are not referenced elsewhere in the codebase.
Regarding the Changelog descriptions, I would appreciate a bit of guidance :)

shruti862 · 2025-01-24T15:47:19Z

Hey @LianaHarris360, I have implemented all the suggested changes and completed the changelog description in the PR template as well. Please let me know if there’s anything else that needs to be addressed.

nucleogenesis

Looks like there are still some issues w/ the variables @shruti862 - seems some of the variables removed are still referenced in the codebase.

Overall, the changes here are on the right track. The changelog LGTM as well. Thanks @shruti862

shruti862 · 2025-01-29T19:40:45Z

@nucleogenesis @LianaHarris360
For the css:

.ui-icon-button-focus-ring {
    background-color: darken($brand-primary-color, 12%);
}

I found three darken utilities: $darken1, $darken2 , and $darken3, but I'm unsure which one best matches darken($brand-primary-color, 12%).

Could you guide me on the best approach to determine the correct match? Is there a predefined scale for these variables, or should I compare them visually with the original color?

lib/keen/styles/variables.scss

akolson · 2025-02-03T14:37:24Z

Hi @shruti862. I have taken some time to review your changes. Great work! However I am unsure about a few things which could be down to my not understanding what needs to be done; My impression from Liana's explanation of the issue is that we should replace all usages of theme dependent scss variables with JS/Vue theme variables in KDS components only.

Wherever those SCSS variables are used in KDS components or styles, replace them with the corresponding JS/Vue theme variables.

That would mean that if the variable is not used in other areas in the codebase, then remove completely, otherwise leave as is. I also don't think the scope of these changes involves updating our KeenUI components themselves. If this is the case, then there's a few places where variables have been removed and yet still in use by Keenui components, for example. @LianaHarris360 could you please confirm if my understanding of this is correct?

LianaHarris360 · 2025-02-03T16:07:49Z

Originally, yes, that was the plan of action, but if there are places where variables that have been removed are still in use in Keen components, I think it does make sense to remove/replace them within this PR.

I do see instances within UiProgressLinear, UiButton, UiIconButton, and UiProgressCircular where they take a color prop and if that prop is "primary", $brand-primary-color is used in the css. In these cases, perhaps a computed property that returns the intended this.$themePalette.primary color should be used instead. I have not dug too deep into this solution for the Keen components and it might be complex to sort out, but if you have any doubts when implementing this, @shruti862, we can help with more guidance.

shruti862 · 2025-02-03T17:41:49Z

@nucleogenesis @LianaHarris360 For the css:
.ui-icon-button-focus-ring {
    background-color: darken($brand-primary-color, 12%);
}
I found three darken utilities: $darken1,$darken2, and $darken3, but I'm unsure which one best matches darken($brand-primary-color, 12%).

Could you guide me on the best approach to determine the correct match? Is there a predefined scale for these variables, or should I compare them visually with the original color?

Hey @LianaHarris360 , I’m currently stuck on this part and would really appreciate your help when you have a moment. Thanks!

LianaHarris360 · 2025-02-03T18:20:38Z

My apologies, I overlooked that comment accidentally. Based on the original percentage, I think $darken1 might be the best choice, although the original is a little darker, it's the closest match. Comparing it visually with the original color is a fine approach to me. When we re-review the pull request and test it in Kolibri, we will be able to confirm if there are any issues with the colors that need to be addressed :)

akolson · 2025-02-04T07:54:37Z

lib/keen/styles/variables.scss

 $ui-input-label-color--hover: black !default;
-$ui-input-label-color--active: $brand-primary-color !default;
 $ui-input-label-color--invalid: $md-red-800 !default;


^ should be replaced to use the KDS theme colors

@akolson, I couldn't find a matching theme color for $md-red-800. As @LianaHarris360 mentioned, we should remove only those SCSS variables that have a corresponding color in KDS. Since this variable doesn’t have a match, I left it as is.

akolson · 2025-02-04T08:07:46Z

lib/keen/styles/variables.scss

 $ui-input-border-color--hover: black !default;
-$ui-input-border-color--active: $brand-primary-color !default;
 $ui-input-border-color--invalid: $md-red !default;
 $ui-input-border-width: 1px !default;
 $ui-input-border-width--active: 1.5px !default;
 $ui-input-border-style--disabled: dotted !default;

 // Input icons
 $ui-input-icon-color: $secondary-text-color !default;


^ can be removed as it not used anywhere

akolson

HI @shruti862! Thanks for the great work. I think we are moving in the right direction. Just a couple of things I picked up during my review;

$ui-input-text-color $ui-input-button-color $ui-input-button-color--hover need to be refactored as well
It would be helpful to pass styling themes as computed props rather than defining them inline. This would greatly improve readability and maintainability.

Thanks again, and be sure to let is know in case you have any questions or comments

lib/KSelect/index.vue

lib/keen/UiIconButton.vue

lib/KSelect/index.vue

LianaHarris360

Thanks for the updates, @shruti862. I just noted a few lingering items to address below.

LianaHarris360 · 2025-03-03T14:52:04Z

lib/KSelect/index.vue

    border-bottom-style: solid;
    border-bottom-width: $ui-input-border-width;
    border-radius: 2px 2px 0 0;
    outline: none;

    &:hover:not(.is-disabled) {
-      border-bottom-color: $ui-input-border-color--hover;
+      border-bottom-color: $ui-input-border-color--hover !important;

      .ui-select-label-text {
        color: $ui-input-label-color--hover;


This line needs the !important at the end as well.

LianaHarris360 · 2025-03-03T15:02:45Z

lib/KSelect/index.vue

@@ -953,29 +959,28 @@
    align-items: flex-start;
    margin-bottom: $ui-input-margin-bottom;
    background: $md-grey-100;
-    border-bottom-color: $ui-input-border-color;
    border-bottom-style: solid;
    border-bottom-width: $ui-input-border-width;
    border-radius: 2px 2px 0 0;
    outline: none;

    &:hover:not(.is-disabled) {


When active, the color changes on hover, but it should remain the same when active and hovered. Perhaps we need to update this pseudo-class selector to ensure the color changes only when hovered, but not when it is disabled and/or active?

This applies for the selector on line 444 in lib/keen/UiTextbox.vue also.

@LianaHarris360, I tried to update the pseudo class to get what is needed but this doesn't work so currently trying other ways :)

Hey @LianaHarris360 , I have corrected the styling of KSelect/index.vue and compared with KSelect from official KDS Page and also changed styling of UiTextbox but quite unsure about this one because I was unable to compare this,
Can you guide me how to review changes made to this component ?

akolson

Hi @shruti862! This is looking great. Liana left some comments to address. Also, there's a few clean up comments I left previously(here and here), but I think we should be good to merge once this feedback is resolved

akolson · 2025-03-04T14:44:32Z

Also bumping this feedback here

shruti862 · 2025-03-04T15:03:16Z

Also bumping this feedback here

I had already worked upon this but for the same reason mentioned above I have to get back the changes made .

…tem into new-changee

removed theme-dependent scss variables

af9ffba

shruti862 closed this Jan 12, 2025

shruti862 reopened this Jan 12, 2025

new-commit

59420a6

MisRob requested review from akolson, nucleogenesis and LianaHarris360 January 15, 2025 18:45

MisRob added the TODO: needs review Waiting for review label Jan 15, 2025

new changes

68ac33e

LianaHarris360 requested changes Jan 22, 2025

View reviewed changes

lib/keen/styles/variables.scss Show resolved Hide resolved

lib/keen/styles/variables.scss Show resolved Hide resolved

shruti862 added 2 commits January 24, 2025 19:26

suggested changes done

049ccf6

fixed lint errors

77f2bec

LianaHarris360 self-requested a review January 27, 2025 15:11

nucleogenesis requested changes Jan 27, 2025

View reviewed changes

new changes

9a62eea

akolson reviewed Jan 30, 2025

View reviewed changes

lib/keen/styles/variables.scss Show resolved Hide resolved

akolson reviewed Feb 4, 2025

View reviewed changes

akolson requested changes Feb 4, 2025

View reviewed changes

AlexVelezLl linked an issue Feb 5, 2025 that may be closed by this pull request

KTextTruncator documentation page lacks an usage section. #899

Closed

AlexVelezLl removed a link to an issue Feb 5, 2025

KTextTruncator documentation page lacks an usage section. #899

Closed

AlexVelezLl linked an issue Feb 5, 2025 that may be closed by this pull request

Remove theme-dependent SCSS variables: Remove variables within KDS files that rely on theming SCSS variables #617

Open

LianaHarris360 self-assigned this Feb 11, 2025

shruti862 requested a deployment to percy_tests February 27, 2025 05:22 — with GitHub Actions Waiting

fixed linting error

7a8e2ec

shruti862 requested a deployment to percy_tests February 27, 2025 05:25 — with GitHub Actions Waiting

LianaHarris360 reviewed Feb 27, 2025

View reviewed changes

lib/KSelect/index.vue Outdated Show resolved Hide resolved

LianaHarris360 reviewed Feb 27, 2025

View reviewed changes

lib/keen/UiIconButton.vue Show resolved Hide resolved

new commit

2231111

shruti862 requested a deployment to percy_tests February 28, 2025 06:06 — with GitHub Actions Waiting

LianaHarris360 requested review from nucleogenesis, akolson and LianaHarris360 February 28, 2025 15:16

LianaHarris360 reviewed Feb 28, 2025

View reviewed changes

lib/KSelect/index.vue Outdated Show resolved Hide resolved

new commit

59c99de

shruti862 requested a deployment to percy_tests February 28, 2025 19:43 — with GitHub Actions Waiting

fixed linting errors

0dcd6e8

shruti862 requested a deployment to percy_tests February 28, 2025 19:59 — with GitHub Actions Waiting

new commit

23adaf8

shruti862 requested a deployment to percy_tests February 28, 2025 21:18 — with GitHub Actions Waiting

LianaHarris360 requested changes Mar 3, 2025

View reviewed changes

akolson reviewed Mar 4, 2025

View reviewed changes

new commits

c5cdb8b

shruti862 requested a deployment to percy_tests March 4, 2025 19:41 — with GitHub Actions Waiting

shruti862 added 2 commits March 5, 2025 01:13

new commits

b8e9538

Merge branch 'new-changee' of github.com:shruti862/kolibri-design-sys…

7f09b00

…tem into new-changee

shruti862 requested a deployment to percy_tests March 4, 2025 19:44 — with GitHub Actions Waiting

new commit

0d4542f

shruti862 requested a deployment to percy_tests March 4, 2025 20:01 — with GitHub Actions Waiting

Merge branch 'develop' into new-changee

1559116

shruti862 requested a deployment to percy_tests March 4, 2025 20:07 — with GitHub Actions Waiting

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed theme-dependent SCSS variables #886

Removed theme-dependent SCSS variables #886

shruti862 commented Jan 12, 2025 •

edited

Loading

LianaHarris360 left a comment

shruti862 commented Jan 22, 2025 •

edited

Loading

shruti862 commented Jan 24, 2025

nucleogenesis left a comment

shruti862 commented Jan 29, 2025

akolson commented Feb 3, 2025

LianaHarris360 commented Feb 3, 2025

shruti862 commented Feb 3, 2025

LianaHarris360 commented Feb 3, 2025 •

edited

Loading

akolson Feb 4, 2025

shruti862 Mar 4, 2025

akolson Feb 4, 2025

akolson left a comment

LianaHarris360 left a comment

LianaHarris360 Mar 3, 2025

LianaHarris360 Mar 3, 2025

shruti862 Mar 4, 2025

shruti862 Mar 4, 2025

akolson left a comment

akolson commented Mar 4, 2025

shruti862 commented Mar 4, 2025

Removed theme-dependent SCSS variables #886

Are you sure you want to change the base?

Removed theme-dependent SCSS variables #886

Conversation

shruti862 commented Jan 12, 2025 • edited Loading

Description

Issue addressed

Before/after screenshots

Changelog

Steps to test

(optional) Implementation notes

At a high level, how did you implement this?

Does this introduce any tech-debt items?

Testing checklist

Reviewer guidance

Comments

LianaHarris360 left a comment

Choose a reason for hiding this comment

shruti862 commented Jan 22, 2025 • edited Loading

shruti862 commented Jan 24, 2025

nucleogenesis left a comment

Choose a reason for hiding this comment

shruti862 commented Jan 29, 2025

akolson commented Feb 3, 2025

LianaHarris360 commented Feb 3, 2025

shruti862 commented Feb 3, 2025

LianaHarris360 commented Feb 3, 2025 • edited Loading

akolson Feb 4, 2025

Choose a reason for hiding this comment

shruti862 Mar 4, 2025

Choose a reason for hiding this comment

akolson Feb 4, 2025

Choose a reason for hiding this comment

akolson left a comment

Choose a reason for hiding this comment

LianaHarris360 left a comment

Choose a reason for hiding this comment

LianaHarris360 Mar 3, 2025

Choose a reason for hiding this comment

LianaHarris360 Mar 3, 2025

Choose a reason for hiding this comment

shruti862 Mar 4, 2025

Choose a reason for hiding this comment

shruti862 Mar 4, 2025

Choose a reason for hiding this comment

akolson left a comment

Choose a reason for hiding this comment

akolson commented Mar 4, 2025

shruti862 commented Mar 4, 2025

shruti862 commented Jan 12, 2025 •

edited

Loading

shruti862 commented Jan 22, 2025 •

edited

Loading

LianaHarris360 commented Feb 3, 2025 •

edited

Loading