Add internal theming support #536
Conversation
|
Thanks @nick2432, this is heading in a good direction. I left some first pointers. |
lib/keen/UiToolbar.vue
Outdated
| @@ -23,16 +21,16 @@ | |||
| </div> | |||
| </slot> | |||
| </div> | |||
| </div> | |||
|
|
|||
| <div class="ui-toolbar__body" :class="{ 'has-brand-divider': hasBrandDivider }"> | |||
There was a problem hiding this comment.
I don't understand how this change relates to the theming issue being resolved in this pull request, could you explain what's its motivation?
| <slot> | ||
| <div v-if="title" class="ui-toolbar__title"> | ||
| {{ title }} | ||
| </div> | ||
| </slot> | ||
| <slot name="navigation" class="ui-toolbar__nav"></slot> |
| @@ -66,7 +64,7 @@ | |||
| }, | |||
| textColor: { | |||
| type: String, | |||
| default: 'black', // 'black' or 'white' | |||
| default: 'default', // 'black' or 'white' | |||
There was a problem hiding this comment.
There are still places in this component that expect textColor be either 'black' or 'white', for example progressColor function or it seems that even some Ui sub-components.
The most straightforward way to deal with it I can think of, while preserving the approach you've used, would be to keep
textColor: {
type: String,
default: 'black', // 'black' or 'white'
}
and then internally map 'black' to tokens.text and 'white' to tokens.textInverted when setting the text color (similarly to your getColor). This won't break other places that still expect these two strings.
| return this.$themeTokens.surface; | ||
| } else { | ||
| const tokens = this.textColor.split('.'); | ||
| return this.$themeTokens[tokens[1]] || ''; |
There was a problem hiding this comment.
I don't understand what you intended to do here, can you explain?
There was a problem hiding this comment.
This is for passing any of the theme tokens listed in the KDS documentation. For example, in the above-given screenshot, I used the themetoken.document token, and it worked.
There was a problem hiding this comment.
textColor="$themeTokens.document" means that you're passing string "$themeTokens.document" rather than variable $themeTokens.document that will evaluate into a color code.
If we were going to pass theme tokens via the textColor prop, then we'd want to accept them like :textColor="$themeTokens.document" which is a shortcut for v-bind that passes variables https://v2.vuejs.org/v2/guide/components-props#Passing-Static-or-Dynamic-Props. Then you wouldn't need
const tokens = this.textColor.
return this.$themeTokens[tokens[1]] || '';
However, looking at the approach you took overall, I'd recommend you accept only 'black' and 'white' strings as textColor (same as previously) but translated them into theme colors as explained here #536 (comment).
lib/keen/UiToolbar.vue
Outdated
| height: $ui-toolbar-height; | ||
| padding-left: rem(16px); | ||
| max-width: 100%; |
There was a problem hiding this comment.
I don't think we need changes of styles other than those related to theming
There was a problem hiding this comment.
This is just one example. I'd recommend cleaning up other places as well. If you saw a bug that you wanted to implement an extra fix for, please always explain the motivation for changes.
|
@MisRob , I accidentally made a lot of changes. I apologize for that. |
|
No problem, @nick2432. Please (1) look at look at guidance here #536 (comment) and (2) clean up all changes that are not related to theming. |
|
I marked this as draft since there's lots of work in progress and it seems we're figuring out a suitable approach. For your work, that doesn't change anything. Please mention me when you finished your updates and I will have a look again. Thank you! |
|
Hi @nick2432, please note that the |
|
Hi @nick2432 I'm sorry but I will close this since meanwhile we needed to do some work around theming and will need to rethink our approach to this issue. |
|
Thank you! |
Description
This pull request enhances the UiToolbar component by integrating theme tokens for background and text colors. Previously, the component used hardcoded colors, and now it dynamically fetches the appropriate color values from the theme tokens based on the component's properties, textColor. This modification ensures that the component adheres to theming conventions and allows for easier customization through theme tokens.
Related Issues
Closes #296
Addresses #PR# HERE
Screenshot