Conversation
|
56040c0 to
a63629b
Compare
4f6a2ed to
3360f15
Compare
matyasf
left a comment
There was a problem hiding this comment.
See my comments.
Also there might be something off with the primary-inverse button's text color in the dark theme:
Similar issue with the inverse ToggleButton:
- You can delete
BaseButton/theme.ts - What about
CloseButton? Will it be converted later? - Please use the new icons in the examples (the README files)
- Please use the new icons in the regression test
matyasf
left a comment
There was a problem hiding this comment.
- The last CloseButton example still uses a old icon
- The samples I linked above still look bad ... what about using a
Viewwith e.g.warningbackground, these color variants are intended for such use case, right? - Please put changes in the upgrade guide (the new sizes and the theme token changes)
| 'x-small': sharedTokens.legacy.spacing.xSmall, | ||
| small: sharedTokens.legacy.spacing.small, | ||
| medium: sharedTokens.legacy.spacing.medium |
There was a problem hiding this comment.
I could not find these in the token changelog. Is it OK, that we use here legacy tokens? Perhaps leave here a comment about why
There was a problem hiding this comment.
it was just a guess, maybe i'll sync with design about this
| ...props | ||
| } = this.props | ||
|
|
||
| const themeOverride = this.props.themeOverride |
There was a problem hiding this comment.
Why is this needed here? why is it not needed for others like IconButton?
There was a problem hiding this comment.
it is done in the iconbutton too. actually the reason i added this is because it is added there too
Reworked Button, CondensedButton, and IconButton theming: - Removed individual component theme.ts files in favor of centralized BaseButton styles - Updated icon sizing inside buttons - Fixed button active state - Added condensed size and disabled variant styling - Set AI button to use transparent background - Migrated button wrapper components to new theme structure
c14d18d to
3ac5f16
Compare
| **/ | ||
| // needed for listing the available theme variables on docs page | ||
| @withStyle(null, generateComponentTheme) | ||
| @withStyle(null, 'BaseButton') |
There was a problem hiding this comment.
can you share the code for both of these example? thanks
There was a problem hiding this comment.
<View display="block" background="primary" padding="small">
<CondensedButton>Click Me</CondensedButton>
</View>
| import { withStyleRework as withStyle } from '@instructure/emotion' | ||
|
|
||
| import generateComponentTheme from './theme' | ||
| import { withStyle } from '@instructure/emotion' |
There was a problem hiding this comment.
really good catch! it is because designers accidentally set the line height in rem, not scale (1.125rem instead of 1.125)
| */ | ||
|
|
||
| import type { BaseButtonTheme } from '@instructure/shared-types' | ||
| import type { NewComponentTypes, SharedTokens } from '@instructure/ui-themes' |
There was a problem hiding this comment.
these tokens from the token studio are not used:
borderRadiusFull
borderRadiusSm
ghostOncolorActiveBackgroundColor
ghostOncolorHoverBackgroundColor
opacityBase
dangerActiveBoxShadow
dangerGhostActiveBoxShadow
primaryActiveBoxShadow
primaryGhostActiveBoxShadow
primaryInverseActiveBoxShadow
primaryInverseGhostActiveBoxShadow
secondaryActiveBoxShadow
secondaryGhostActiveBoxShadow
successActiveBoxShadow
successGhostActiveBoxShadow
There was a problem hiding this comment.
note, these were removed:
dangerActiveBoxShadow
dangerGhostActiveBoxShadow
primaryActiveBoxShadow
primaryGhostActiveBoxShadow
primaryInverseActiveBoxShadow
primaryInverseGhostActiveBoxShadow
secondaryActiveBoxShadow
secondaryGhostActiveBoxShadow
successActiveBoxShadow
successGhostActiveBoxShadow
-opacityBase should be used
- these 2 will be checked by design:
ghostOncolorActiveBackgroundColor
ghostOncolorHoverBackgroundColor
borderRadiusSmshould be used for the new consended sizes (condensedSmall, etc)borderRadiusFullis for circle icon btn
| size?: 'small' | 'medium' | 'large' | 'condensedSmall' | 'condensedMedium' | ||
|
|
There was a problem hiding this comment.
In Basebutton a size and color combination of like this below results some invisible button when it is not hovered. (Not sure if it a valid use case tho)
<BaseButton size="condensedSmall" color="primary">CS</BaseButton>
There was a problem hiding this comment.
i guess this is not a valid state so i don't know how big of a deal this is. cc @hajnaldo
There was a problem hiding this comment.
@hajnaldo confirmed this combination should never exist
| label: 'closeButton', | ||
| zIndex: componentTheme.zIndex, | ||
| zIndex: 1, |
There was a problem hiding this comment.
Why isn't this tokenized anymore?
There was a problem hiding this comment.
@hajnaldo confirmed it's ok if it's hardcoded. we can change later if the need arises
| <IconButton withBackground={false} withBorder={false} color="primary-inverse" screenReaderLabel="Delete tag" margin="large"> | ||
| <IconXSolid /> | ||
| <XInstUIIcon /> | ||
| </IconButton> |
There was a problem hiding this comment.
i've missed this. this example should be removed since this is not a valid design anymore
| Button: BaseButtonTheme | ||
| CloseButton: CloseButtonTheme |
There was a problem hiding this comment.
import type { CloseButtonTheme } from '@instructure/shared-types' was removed from styles.ts of CloseButton, why was this change needed?
There was a problem hiding this comment.
there is not closebuttontheme anymore, it does not have its own variables




INSTUI-4787
Summary
Migrates Button, CondensedButton, and IconButton to the new v12 centralized theme system.
theme.tsfiles from Button, CondensedButton, and IconButton (they re-exported BaseButton's theme) — theming is now handled centrally viawithStyle(null, 'BaseButton')withStyleReworktowithStyleand from legacygenerateComponentThemeto the new token-based approach (NewComponentTypes/SharedTokens)opacity: 0.5)condensedSmallandcondensedMediumrenderIconWithPropswith proper size mapping per button size::beforepseudo-element mask technique instead of relying on background gradient as borderdisplay: blocktodisplay: flexwithalignItems: centerfor proper vertical alignmentdarken/lightencolor utils with dedicated theme tokens for hover/active statesTest plan
condensedSmall,condensedMedium) render at the correct compact heights