fix!: Increase specificity on button styles #198
Conversation
src/bundles/AiDrawer/AiDrawer.tsx
Outdated
| flexShrink: 0, | ||
| })) | ||
| const CloseButton = styled(ActionButton)(({ theme }) => | ||
| withStyleOverrides({ |
There was a problem hiding this comment.
For my knowledge!
This withStyleOverrides helper ensures that when you intentionally override design-system button styles, your styles always take effect — even in environments with strong competing CSS (like Canvas LMS). Right?
There was a problem hiding this comment.
Not necessarily, but they will ensure the design system button styles themselves, which also now use &&. This raises specificity to ensure styles should not in most cases be unintentionally applied, but without raising it so high that the only way to override is with !important.
| RiDeleteBinLine, | ||
| RiTestTubeLine, | ||
| } from "@remixicon/react" | ||
|
|
There was a problem hiding this comment.
lets not remove this lets keep it here what do you think?
There was a problem hiding this comment.
We don't have any rules for newlines here, so I'm indifferent, however I don't see any grouping between the imports above and below to need one here.
| * with potentially conflicting buttons styles from the parent | ||
| * page in a consuming application. | ||
| */ | ||
| export const PageStyleResistance: Story = { |
There was a problem hiding this comment.
Just to understand this!
- PageStyleResistance is a Storybook scenario that:
- Applies intentionally conflicting global button styles
- Renders the design-system button inside that environment
- Shows/proves that design-system styles still win
i am thinking of it like CSS unit testing.
Am i right?
There was a problem hiding this comment.
Yes, all correct. Not so much unit testing, though could be viewed as a visual form of unit testing - the Storybook stories provide a way to present the components in isolation with in their various variants and explore their behavior. In this case, we're validating correct render by adding intentionally conflicting styles in the controlled environment.
src/components/Button/Button.tsx
Outdated
| ...sx, | ||
| }) | ||
|
|
||
| return css` |
There was a problem hiding this comment.
Great job increasing style-specificity using && — this will prevent LMS global button CSS from leaking into our components. This is key to ensuring Button styles don’t get overridden by host app CSS (e.g., LMS global button styles).
| import type { ButtonLinkProps, ButtonProps } from "../Button/Button" | ||
| import { css } from "@emotion/react" | ||
| import type { Theme } from "@mui/material/styles" | ||
| import { withStyleOverrides } from "../../utils/styles" |
There was a problem hiding this comment.
Question / clarity:
Can we document the withStyleOverrides() helper inside the design system README so future contributors know when to use it?
There was a problem hiding this comment.
We tend to document usage notes alongside the code itself (see src/utils/styles.ts), though this could be surfaced more prominently.
src/components/Button/Button.tsx
Outdated
| variant: ButtonVariant, | ||
| colors: Theme["custom"]["colors"], | ||
| ) => { | ||
| if (variant === "primary") { |
There was a problem hiding this comment.
Right now, variantStyles and sizeStyles are implemented as big conditional functions with multiple if/else branches. This works well today, but over time it makes maintenance harder because:
- Themes and button variants grow → more conditions to manage
- Styling rules are scattered across logic instead of a central config
- It’s harder for designers/devs to customize in one place
Refactoring these into a theme token map (design-token-driven styling) would allow us to declare variants & sizes in a central theme object instead of conditional JS. This shifts styling from logic → configuration.
if (variant === 'primary') {
return { backgroundColor: colors.mitRed }
} else if (variant === 'secondary') {
return { borderColor: colors.silverGray }
}We could move it into theme tokens:
theme.components.button = {
variants: {
primary: {
backgroundColor: colors.mitRed,
color: colors.white,
border: 'none'
},
secondary: {
backgroundColor: 'transparent',
color: colors.red,
border: `1px solid ${colors.silverGray}`
}
},
sizes: {
small: { padding: '8px 12px', ...theme.typography.buttonSmall },
medium: { padding: '11px 16px', ...theme.typography.button },
large: { padding: '14px 24px', ...theme.typography.buttonLarge }
}
}what do you think?
There was a problem hiding this comment.
We do provide a theme for common values, see https://github.com/mitodl/smoot-design/blob/main/src/components/ThemeProvider/ThemeProvider.tsx. These tie in with the naming in the Figma designs. For this case, the values are specific to the button component and self contained there - I'm not sure moving them out to the theme would add value. I'm not against specifying styles declaratively for the simpler cases, though we have places where the variant styling has fairly complex conditional combinations, context dependent styling and nested selectors where handling token objects will quickly become unwieldy - https://github.com/mitodl/smoot-design/blob/main/src/components/Input/Input.tsx#L54-L183 for example.
arslanashraf7
added
Waiting on Author
and removed
Needs Review
|
@jonkafton Regarding the styling issues with OpenEdx in general:
Increasing our specificity helps with (1) but not (2). In slack, you mentioned layers as a possible solution. I don't think layers will help us (at least not without !important in each style) because
To deal with issue (2), I think we need some css-resets, but they can only affect our own components, not openedx. Regarding specificity boosting: It would be good to avoid increasing the specificity in Learn unnecessarily. What if
Boy it would have been nice if the shadow dom approach had worked...totally isolated styling. Too bad it messed up keyboard controls. |
|
I'm struggling to find a good general solution.
For this case we need to increase specificity. The issue with doing that is we are introducing a breaking change for consuming apps that override Smoot Design styles intentionally - they would also need to increase specificity. The
No, the primary intention in this PR is to address the conflicting action button styles in Canvas, though we are in need of a general solution to avoid chasing issues. We do need reset styles, though these also come with the specificity issue. Canvas, for example, applies styles to Our options:
The |
… and hook to use for selector targeting
|
Change of tack - implements option 4 in comment above.
|
…icity on reset styles
…le). Remove now redundant Button resets
|
Pushed an additional change:
|
There was a problem hiding this comment.
(Requesting changes as a signal not to merge yet, since this was approved then changed a lot).
First: Do you know why the changes to main.ts are being picked up? That wasn't changed on your branch as far as i know. It's already merged.
Second: Really liked the new StyleIsolation tests! I'm still a bit wary of how far-reaching the code changes are. I asked ChatGPT for suggestions on how to solve this problem, assuming we are using emotion, and it suggested using a stylis plugin (emotion's css preprocessor).
Here's the discussion: https://chatgpt.com/share/691e4e95-ea74-800e-861d-691cfe03deb1
Did you look into a stylis plugin at all? It seems like a promising approach.
NOTES:
- The code ChatGPT gave doesn't really work. A working version of the plugin is below.
- In general, this approach seems a lot nicer... we wouldn't need to change our usage of
styledat all. No need foruseStyleIsolation. - That said, it's pretty low-level in emotion guts, I think, and I don't have a lot of experience with csss-preprocessors. (Though we can use the plugin only conditionally, which is nice.)
Here's a plugin that seemed to work well for me:
import type { StylisPlugin, StylisElement } from "@emotion/cache"
/**
* NOTE
* I would suggest just using a static class name like `.Mit-isolated`
* No need for context.
*/
export const increaseSpecificity = (scope: string): StylisPlugin => {
return (element: StylisElement) => {
if (element.type === "rule" && Array.isArray(element.props)) {
console.log(element.props)
element.props = element.props.map((sel: string) => {
if (
sel.startsWith("@") ||
sel.startsWith(":root") ||
sel.includes(`${scope}${scope}${scope}`)
) {
return sel
}
return `${sel}, ${scope}${scope}${scope} ${sel}`
})
}
}
}Open questions:
- Does this work? It seemed to. (Most of my testing was figuring out we needed to exclude boosting specificity of nested selects over and over again)
- Possibly need a way to ensure the CSS resets don't have their specificity boosted?
- If we provide a plugins array, does that override the default? (E.g., are there default emotion plugs we'd need to re-enable?
- Note: MUI has
NextJsAppRouterCacheProvider, but that also takes a plugins array.
- Note: MUI has
- One day in the future, we may want to replace emotion with MUI's Pigment. Unsure if there's a similar approach we could take in that world. (Though I imagine they'll support emotion for a long time, so you could image we use Pigment in Learn but Emotion in canvas/openedx.)
|
@jonkafton One other thing... This isn't a breaking change anymore, right? Should change PR title. |
I reset to main but wanted the history as it ties to the PR thread. Git is producing the diff based on the history even though the content is has not changed. A rebase should fix, though I tend to avoid.
It would definitely be good to reduce complexity. If I'm reading correctly, the plugin approach increases the specificity of all selectors, which we weren't wanting to do unless we are applying reset CSS. If the preprocessor also increases specificity when The thinking for the current change:
Running at AST level definitely helps us here a lot.
Given the amount of time this has been in progress with respect to the request being to fix a bad background on a button, we might want to test out the above and make the change in a follow up.
It is aiming not to be, but given the breadth it should be at least a minor bump. I don't think Smoot is used anywhere with auto upgrade within minor range (or auto upgrade at all), but we'll want to take extra care when upgrading. |
What are the relevant tickets?
Closes https://github.com/mitodl/hq/issues/9059
BREAKING CHANGE:
This boosts style specificity to ensure page styles are not applied unintentionally. This will impact consuming apps applying intentional styles. A helper method is provided on the Smoot Design API to facilitate upgrade.Component style specificity is not increased by default, however the AiChat and AiDrawer bundles are wrapped in a style isolation wrapper that applies reset styles scoped to the container and increases Smoot Design style specificity on Input and Button components. These should be checked carefully when upgrading in projects.See comment below.
Description (What does it do?)
<StyleIsolation>wrapper that resets common text input and button styles.<StyleIsolation>to the AiChat and AiDrawerManager bundles (AiChat at root and AiDrawerManager within the Drawer, wrapping content).How can this be tested?
Run storybook with
yarn startRun the bundle preview with
yarn bundle-previewVisit the AiChat and AiDrawer manager demos at http://localhost:3000/bundle-ai-chat-canvas-styles and http://localhost:3000/bundle-ai-drawer-canvas-styles.
These import the conflicting stylesheets from Canvas. Check that feedback buttons and textarea input render as expecte.
Additional Context
Uses
&&specificity stacking, which result in generated classnames being chained and repeated, e.g..css-1s87gzz.css-1s87gzz..css-1s87gzz. ThewithStyleOverrides()helper wraps styles with the same, ```&& { ...styles }`.