-
Notifications
You must be signed in to change notification settings - Fork 81
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
Finalize theme token updates #551
Finalize theme token updates #551
Conversation
Thanks @LianaHarris360. Redirecting here the conversation about the changelog from the closed PR: Thank you, new updates make sense. I see one place that could deserve more detail in my view When you say
Could you list new labels and before/after of renamed token labels here? So we can find them in the products easily and rename them without the need to dig into this PR's diff. These are changes in public API and we will be specific in the release notes (this is different from just tweaking an existing color which we don't need to document in such detail.
Generally, you could just imagine that you're someone who's not familiar with the code diff in detail and tried to introduce new theming to Kolibri/Studio/KDP. Anything that would be useful for you to know will serve as good guidance :) Thank you. This will be helpful later on as we are updating Kolibri, Studio, and KDP. |
FYI I'm gradually reviewing this as I'm working on rebranding Kolibri, so it may take a while to post final feedback, but it's in progress. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, @LianaHarris360, thank you so much for the quick and thorough work on this.
I spoke briefly with @MisRob and I think for the rest of the week, perhaps we can keep this open in case there are any changes to be made here that are discovered in the updates to Kolibri and Studio, and they can just be made directly here, so I am commenting rather than approving. Managing this will take some working cooperatively and communication, but I think we can do it. :)
Overall though, great start. For those who will be doing the updates in our products, I think this is ready to work from at least to do some next steps. Please let me know if you have any questions, either here or in the slack thread in #product.
Hi, I did first review and overall great work @marcellamaki @LianaHarris360, thank you. See notes below from the review and also my parallel work of using it in Kolibri. It's work in progress so it's possible that I will post some more over time, but I think this could be all when it comes to high-level: (1) I located more breaking changes that are not in the changelog yet. @LianaHarris360 I needed to note them down for myself when upgrading Kolibri, so I hope it's fine if I just updated the changelog directly rather than referencing it. (2) One high-level issue that's blocking some Kolibri work is removal of yellow color from the full palette. The only way how to use yellow now is via (3) Relatedly to (2), I think that except of tokens that are related to brand (such as tokens.primary, tokens.primaryDark, ...), tokens need to be created via palette rather than brand colors. For example, Here, I only pointed to a few places I looked into in more detail, but I think there are some more. @LianaHarris360 could you please revisit it as a whole and make sure that there's distinction between blue and yellow for brand and palette? |
Also in regards to (2), the yellow we need in the palette is this one. I think that's the same yellow as the one used for brand secondary, but please double-check https://www.figma.com/file/p7aOP2MBv0SED1uhaPwqEd/Product-rebrand?node-id=766%3A13416&mode=dev Note that Figma has Here's one of the relevant Kolibri places https://github.com/learningequality/kolibri/blob/release-v0.16.x/kolibri/plugins/device/assets/src/views/DeprecationWarningBanner.vue#L10-L14 |
primaryDark: 'brand.primary.v_1100', | ||
secondary: 'brand.secondary.v_1000', | ||
secondaryDark: 'brand.secondary.v_1100', | ||
appBar: 'brand.secondary.v_800', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we please document all tokens on Colors page such as appBar
and possibly others that are missing? I can't find it at https://deploy-preview-551--kolibri-design-system.netlify.app/colors/# It seems some of them were forgotten before, it'd be helpful to include them now as we're upgrading though.
I'd like to ask for updating
grey.v_50 .
This will change the darker gray area in the bottom half in Kolibri to match the new lighter upper part. |
If it's easier for you to update the changelog directly, I don't mind at all! I will go over the changes, document the missing colors, and update this PR based on your review notes. |
Thank you, I updated it in the description. Yes, it would be nice to keep it up to date as we're doing more changes here. |
New changes are looking fine, thank you @LianaHarris360 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @LianaHarris360, great work. The newest additions are looking fine to me and I think we resolved all outstanding issues. I will merge in preparation for the release. If there's some other work needed, we can have a new PR.
Description
This PR updates the the brand token colors, names, and token mapping within components and mixin references and expands upon the initial token to hex code mapping work done by @marcellamaki
Issue addressed
Addresses #545
Before/after screenshots
Changelog
brand
colors,palette
colors, andtoken
s.palette
colors:purple
,deeppurple
,indigo
,brown
,cyan
,teal
,lightgreen
,lime
,amber
,deeporange
,bluegrey
palette.grey
scales:v_300
,v_500
,v_700
,v_900
brand
andpalette
scales (exceptpalette.grey
):v_50
,v_100
,v_300
,v_500
,v_700
,v_900
exercise
,video
,audio
,document
,html5
,slideshow
appBarFullscreen
,appBarFullscreenDark
,linkDark
palette
colors look differently<body>
background color changed fromgrey.v_100
to lightergrey.v_50
.palette
color with a newtoken
that describes function of the color better). If you usegenerateGlobalStyles
that generates background color for<body>
and usegrey.v_100
in some components to match the background color, you may need to update it togrey.v_50
.(optional) Implementation notes
This line was removed from the KDS Colors Documentation Page because the mentioned colors were removed:
KContentRenderer was removed from the table of contents because it displayed an error page
Testing checklist
Reviewer guidance
After review
CHANGELOG.md