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

Update KDS Theme Tokens #782

Merged

Conversation

AlexVelezLl
Copy link
Member

@AlexVelezLl AlexVelezLl commented Sep 16, 2024

Description

Update KDS Theme Tokens to the latest specs in #775.

Issue addressed

Closes #775.

Before/after screenshots

Before After
image image
image image
image image

Changelog

Steps to test

  1. Look through components and see that there is no regression regarding colors
  2. Check that all colors have been changed following the colors map in Update KDS Theme Token values #775.

Implementation notes

At a high level, how did you implement this?

Searched for all v_* colors and replaced all of them but gray.v_400 and gray.v_800 which remained the same.

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

@AlexVelezLl AlexVelezLl changed the base branch from develop to release-v4 September 20, 2024 18:44
@AlexVelezLl AlexVelezLl linked an issue Sep 24, 2024 that may be closed by this pull request
@marcellamaki marcellamaki self-assigned this Sep 24, 2024
Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

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

Thanks Alex, this looks great. I added just a couple of very small comments. I'm going to try testing this out with one of our custom themed plugins to confirm that everything is working, and then if everything looks good with that I'll approve

</li>
<li>
Option text hover color:
<DocsInternalLink code text="palette.grey.v_100" href="/colors#palette-grey-v_100" />
<DocsInternalLink code text="palette.grey.v_200" href="/colors#palette-grey-v_100" />
Copy link
Member

Choose a reason for hiding this comment

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

I think these should both be v_200? Although looking at the docs, I'm not sure that this is meaningfully displayed to the user (not your changes, just in general I'm not sure how helpful it is or how much of a difference it makes)

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops 😅 Fixed it!

Although looking at the docs, I'm not sure that this is meaningfully displayed to the user

Yes, I think the components specs pages are a little outdated. I just saw that in Appbars for example, we are still using the old palette for the examples.

<p style="margin-left: 60px">
</p>
<DocsColorBlock name="palette.green.v_1000" />
<DocsColorBlock name="palette.green.v_1100" />
<DocsColorBlock name="palette.green.v_500" />
Copy link
Member

Choose a reason for hiding this comment

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

let's replace the

<p style="margin-left: 60px">
    …
</p>

with
<DocsColorBlock name="palette.green.v_400" />

(not strictly in scope of the palette changes, but I think there isn't really a need to skip the 400 value. I think the "break" was more about the larger gap in between)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done! I also updated the paragraph above "and segment colors into brightness levels, named <code>v_100</code>, <code>v_200</code>, <code>v_300</code>, <code>v_400</code>, <code>v_500</code>, <code>v_600</code>:"

@AlexVelezLl
Copy link
Member Author

Thank you @marcellamaki! Just pushed the changes (:

Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

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

Looks great, thank you @AlexVelezLl.

When testing with our "cookiecutter" theme plugin, the current version of Kolibri develop breaks, and this fixes it! (Don't mind the ugly test colors :) )

Before After
Screenshot 2024-10-01 at 1 23 48 PM Screenshot 2024-10-01 at 1 41 31 PM

@AlexVelezLl
Copy link
Member Author

Thank you @marcellamaki!

@AlexVelezLl AlexVelezLl merged commit 93119bb into learningequality:release-v4 Oct 1, 2024
8 checks passed
@AlexVelezLl AlexVelezLl deleted the update-theme-tokens branch October 1, 2024 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update KDS Theme Token values
2 participants