-
Notifications
You must be signed in to change notification settings - Fork 175
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
Studio: Audit and fix theme token updates against KDS #4459
Studio: Audit and fix theme token updates against KDS #4459
Conversation
Fixes "Error: Can't resolve 'kolibri-design-system/lib/utils/i18n'" caused by removal of this file in KDS v2.0.0 by copying the removed file content directly to Studio.
Remove deprecated `value` prop in favor of the new `buttonValue` prop that's supposed to replace it in `KRadioButton`.
color: { | ||
type: String, | ||
default: 'secondary', | ||
}, |
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.
I added a color
prop here as there are instances where the MainNavigationDrawer
top needs to match with the AppBar
color since its not always yellow
. We actually do have a white app bar too and this prop provides the much needed flexibility.
contentcuration/contentcuration/frontend/shared/vuetify/theme.js
Outdated
Show resolved
Hide resolved
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.
Hi @akolson - this looks like good work! One thing that needs updating, which wasn't fully outlined, is the "immersive" app bars, or full screen modal app bars, should all be black, rather than blue. I just double checked with Jessica. Some of this might be token swapping, and perhaps some of it is hex codes - I'm not sure.
Other than that, I don't see any critical blockers right now.
I did notice one KDS update needed for the loaders, but I'll follow up with @LianaHarris360 about that in KDS :)
Thanks @marcellamaki. I think we should be good now; |
Oops, it's a vuetify thing, not a KDS thing! So, there are several places where (Sorry I thought I was going to make this edit in time before you pushed changes but I didn't quite make it ) |
@marcellamaki I think the figma designs spec mentions using black if I recall correctly |
<img | ||
:height="`${height}px`" | ||
width="auto" | ||
:src="require('shared/images/kolibri-logo.svg')" |
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.
Any other usages of this file? Is it safe to delete?
@akolson The icon indicating resources are coach content seemingly isn't using the correct color. The color comes from CSS vars, so it seems to possibly be set through some explicit CSS. |
@@ -73,7 +76,11 @@ | |||
</VListTile> | |||
</VList> | |||
<VContainer> | |||
<KolibriLogo :height="75" /> | |||
<KLogo | |||
altText="Kolibri logo" |
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.
Seems like this alt text should be translated. Although for the rebranding release, I think we're trying to avoid translating things. It also sucks I don't think we can have KLogo
just have this internally.
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.
Approved! Merging so we can create the release PR
Things for followup:
- KLogo alt text
channelHighlightDefault
color- whether there are other usages of
shared/images/kolibri-logo.svg
that need replaced, and if not can we delete the file
Summary
Description of the change(s) you made
Updates Studio with KDS theming updates introduced in learningequality/kolibri-design-system#551.
Manual verification steps performed
Screenshots (if applicable)
Does this introduce any tech-debt items?
Reviewer guidance
How can a reviewer test these changes?
Are there any risky areas that deserve extra testing?
Development
QA
References
Closes #4439
KDS PR: learningequality/kolibri-design-system#551
Designs: https://www.figma.com/file/p7aOP2MBv0SED1uhaPwqEd/Product-rebrand?type=design&node-id=426-7545&mode=design
Comments
Contributor's Checklist
PR process:
CHANGELOG
label been added to this PR. Note: items with this label will be added to the CHANGELOG at a later timedocs
label has been added if this introduces a change that needs to be updated in the user docs?requirements.txt
files also included in this PRStudio-specifc:
notranslate
class been added to elements that shouldn't be translated by Google Chrome's automatic translation feature (e.g. icons, user-generated text)pages
,components
, andlayouts
directories as described in the docsTesting:
Reviewer's Checklist
This section is for reviewers to fill out.
yarn
andpip
)