AppBar: Use appBarText for text/icon colors #12937
Conversation
- NavBar & NavbarLink now take & drill a `textColor` prop - In AppBar, the new token `appBarText` is passed to `textColor` - In AppBar, it is also used as the default color for icons, the page title, and the user's info This is done to permit plugins to customize the text in the AppBar so that they can ensure proper contrast with their chosen `appBar` token's value, which is used as the background.
| @@ -10,9 +10,12 @@ | |||
| <UiToolbar | |||
| :removeNavIcon="showAppNavView" | |||
| type="clear" | |||
| textColor="black" | |||
| :textColor="$themeTokens.appBarText || $themeTokens.text" | |||
There was a problem hiding this comment.
What the usage looks like in the plugin:
Build Artifacts
|
…, bg, and textColor
nucleogenesis
force-pushed
the
themeTokens-appBarText-for-is
branch
from
b1e5b68 to
4c0bee9
Compare
There was a problem hiding this comment.
Hi @nucleogenesis - for some reason the top navigation bar is not displayed at all:
2024-12-13_16-12-32.mp4
There are multiple TypeError: Cannot read properties of undefined (reading 'appBar') errors in the console:
| @@ -57,11 +57,15 @@ | |||
| required: true, | |||
| validator: validateLinkObject, | |||
| }, | |||
| textColor: { | |||
There was a problem hiding this comment.
Could we avoid prop drilling here, and just reference the value directly? NavbarLink is only used in the Navbar component, and we remove it from the public API completely in develop, so we can just set it directly to the value we want.
There was a problem hiding this comment.
Good catch - just updated this
| @@ -81,6 +82,10 @@ | |||
| return values.every(value => value.link.name); | |||
| }, | |||
| }, | |||
| textColor: { | |||
There was a problem hiding this comment.
Similarly here, Navbar is only used in AppBar, and is also removed from the public API in develop, so I think we can just reference theme variables directly.
|
@pcenov thank you and apologies for the oversight on my end here. I had my devserver running while working on it but didn't see the top bar disappear while making the updates 😅 I've pushed some updates, so should be ready for re-review. |
There was a problem hiding this comment.
Thanks @nucleogenesis - no regressions observed while manually testing!
| spec: { | ||
| background: { | ||
| type: String, | ||
| default: themeTokens().appBar, |
There was a problem hiding this comment.
We don't need to update it here - but if we are going to remove it from the themeTokens in KDS, might just want this to map to something more directly.
| @@ -29,6 +29,7 @@ | |||
|
|
|||
| import { validateLinkObject } from 'kolibri.utils.validators'; | |||
| import useKResponsiveWindow from 'kolibri-design-system/lib/composables/useKResponsiveWindow'; | |||
| import themeConfig from 'kolibri.themeConfig'; | |||
There was a problem hiding this comment.
Note to self - this import path will need to be updated when we merge up.
github-actions
bot
added
the
DEV: backend
Summary
textColorpropappBarTextis passed totextColorThis is done to permit plugins to customize the text in the AppBar so that they can ensure proper contrast with their chosen
appBartoken's value, which is used as the background.References
Fixes learningequality/kolibri-instant-schools-plugin#205
Reviewer guidance
QA
Are there any regressions in the default Kolibri bevhaior for the top yellow AppBar? Check each plugin and any sub-routes you can.
The fixes that this allows will be tested upon rebuilding the VF IS plugin w/ this patched on Kolibri.
Code
appBarTextis used so having them defined in KDS may not be necessary (cc @AlexVelezLl)appBarTextkey in thetokenMappingsection of the theme definition in the Instant Schools plugin and w/ that build, all of the colors are shown as expected. Should this be part of the theme itself - or does adding to that mapping jive w/ the way we want to use the theme machinery?