-
Notifications
You must be signed in to change notification settings - Fork 311
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
feat: add theming feature to elements #3821
base: master
Are you sure you want to change the base?
Conversation
959d43a
to
1d83ce3
Compare
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.
is it safe to delete this https://github.com/box/box-ui-elements/blob/master/src/styles/theme.js or at least remove it from storybook or mark it as deprecated this way the customer is not confused?
@@ -1269,6 +1272,7 @@ class ContentUploader extends Component<ContentUploaderProps, State> { | |||
<TooltipProvider> | |||
{useUploadsManager ? ( | |||
<div ref={measureRef} className={styleClassName} id={this.id}> | |||
<ThemingStyles selector={`#${this.id}`} theme={theme} /> |
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.
Is there a way we can have a VRT for this since we know this component should be able to apply a theme. at least we can verify the feature is working at a high level?
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.
yeah I have a separate ticket for adding demos and I can include the VRTs in that
no this theming is still used in EUA and I'm guessing Admin console as well. it's how components that haven't been migrated to Blueprint use custom branding in the webapp. |
I think we should ensure customers dont try to use this one by at least saying its been deprecated or removing the story for it. what do you think? |
yeah I can remove the storybook and styleguidist related to theming. i'm not sure about deprecation warnings though since it'll also effect EUA developers for however long. we'll probably have to do that closer to when EUA has fully migrated to blueprint. btw this new feature doesn't replace the existing theming stuff in buie, they handle separate use cases. the existing theme work is for internal apps while the new theme work is for ui elements |
then just removing it from the documentation is enough |
This change adds a new component
ThemingStyles
to the root of the elements to enable customers to configure theming for components that utilize Blueprint.ThemingStyles
expands on Blueprint'sBrandingStyles
implementation and allows developers to overwrite any blueprint tokens using a configuration object.theme
object which will be transformed into CSS properties that are scoped to the element:The structure of the passed theme configuration object is not strict and allows for different structured inputs providing the same output. The intended structure is based on how our internal design team defines the levels of the tokens but users can also provide a flattened structure if preferred. DevRel will provide a template theme configuration that customers can modify in order to utilize this feature. In the future, there are goals to build a theme builder that will help developers build a config through a user interface.
Reference: https://www.npmjs.com/package/@box/blueprint-web-assets
/@box/blueprint-web-assets/tokens/tokens-css-vars.scss
/@box/blueprint-web-assets/tokens/tokens.scss