Remove dark: style overrides from base button#22
Merged
Conversation
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
ui-storybook | 823e296 | Visit preview | Jun 04 2025, 01:28 AM |
Contributor
|
I am not a fan until the styles.css file is unified because it will have downstream effects to lilypad app. Is this urgent? I have a PR that is switching to using bun with mirascope-ui. We can then test these changes once that PR is merged in to make sure there are no regression issues |
Collaborator
Author
|
It's important that this get merged eventually (since this is the version of the code that I currently have used/committed in mirascope/website), however it's not urgent. I'll just ask that we don't merge any further changes to button outside of this PR, so that whatever we finalize on, I can easily make sure it's well integrated to mirascope/website too. |
brenkao
approved these changes
Jun 5, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR removes the
dark:style overrides from base button.The way I am using the UI system, there is generally not a need for
dark:overrides, because the color variables themselves update in dark mode. For example, indark, the accent color shifts to better harmonize with a dark background.Specifying
dark:classes on the base button disrupts this usage, since thedark:marker gets additional specificity and may interfere with overrides that are being applied, basically forcing the "special case dark mode" logic to bubble up to components that inherit it.As a specific case, consider the
TabsTrigger, which inherits button styling:In light mode, the
data-[state=active]:bg-primaryoverrides thehover:from button, which is desirable — we don't the active tab to become less intense when hovered. However, in the current implementation, thedark:hover:bg-accent/50is more specific and overrides it, which means that tabs would need to havedark:data-[state=active]:bg-primarytoo, and so forth.Hence, while it's still fine for downstream users to set custom
dark:overrides if desired, I don't want to do so in the base ui components.To see this in action, try hovering over the active tab in dark mode on the tab storybook (https://ui.mirascope.com/?path=/story/ui-tabs--default) and compare with the better behavior from this PR.