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

…into develop

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!

LianaHarris360 · 2025-01-22T16:41:03Z

lib/keen/styles/variables.scss

@@ -33,9 +33,8 @@ $ui-input-height: rem(32px) !default;
 $ui-input-margin-bottom: rem(16px) !default;

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


Even though it has been removed, there are still a couple of places in the file lib/KSelect/index.vue where ui-input-label-color is used, this could be causing some of the tests to fail.

~~usages of $ui-input-label-color still exist in KSelect~~ Just rechecked this; we should be good

LianaHarris360 · 2025-01-22T16:49:31Z

lib/keen/styles/variables.scss

 $ui-input-label-color--hover: black !default;
-$ui-input-label-color--active: $brand-primary-color !default;


ui-input-border-color--active is still in use within lib/KSelect/index.vue as well

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?

akolson · 2025-01-30T19:53:50Z

lib/keen/styles/variables.scss

@@ -49,17 +48,14 @@ $ui-input-text-color--disabled: $disabled-text-color !default;
 $ui-input-text-color--invalid: $md-red !default;


We can remove this. It's not being used anywhere.

Thank you for letting me know, I will surely remove this in my next commit.

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 · 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

shruti862 added 3 commits December 8, 2024 17:03

updated lib/KListWithOverflow.vue and lib/KBreadcrumbs.vue

7419e1a

Merge branch 'develop' of github.com:shruti862/kolibri-design-system …

070bd22

…into develop

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

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

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

Open

AlexVelezLl removed a link to an issue Feb 5, 2025

KTextTruncator documentation page lacks an usage section. #899

Open

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

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

LianaHarris360 Jan 22, 2025

akolson Feb 4, 2025 •

edited

Loading

LianaHarris360 Jan 22, 2025

shruti862 commented Jan 22, 2025 •

edited

Loading

shruti862 commented Jan 24, 2025

nucleogenesis left a comment

shruti862 commented Jan 29, 2025

akolson Jan 30, 2025

shruti862 Jan 31, 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

akolson Feb 4, 2025

akolson left a comment

		$ui-input-label-color--hover: black !default;
		$ui-input-label-color--active: $brand-primary-color !default;

		@@ -49,17 +48,14 @@ $ui-input-text-color--disabled: $disabled-text-color !default;
		$ui-input-text-color--invalid: $md-red !default;

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

LianaHarris360 Jan 22, 2025

Choose a reason for hiding this comment

akolson Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

LianaHarris360 Jan 22, 2025

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 Jan 30, 2025

Choose a reason for hiding this comment

shruti862 Jan 31, 2025

Choose a reason for hiding this comment

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

akolson Feb 4, 2025

Choose a reason for hiding this comment

akolson left a comment

Choose a reason for hiding this comment

shruti862 commented Jan 12, 2025 •

edited

Loading

akolson Feb 4, 2025 •

edited

Loading

shruti862 commented Jan 22, 2025 •

edited

Loading

LianaHarris360 commented Feb 3, 2025 •

edited

Loading