Proposal: Change styles structure #2160
Merged
Conversation
This file contains 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
|
|
Size Change: -174 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
| [inputOptionWedge]: showWedgeProp, | ||
| [getWedgeStyles(renderedContext, state, theme)]: shouldRenderWedge, | ||
| [getHoverStyles(renderedContext, theme)]: | ||
| !disabled && isInteractive && state !== State.Highlight, |
There was a problem hiding this comment.
nit: I would extract his into a separate shouldShowHoverStyles var or something like that
Comment on lines
90
to
91
| [inputOptionWedge]: showWedgeProp, | ||
| [getWedgeStyles(renderedContext, state, theme)]: shouldRenderWedge, |
There was a problem hiding this comment.
Why are these separate?
There was a problem hiding this comment.
Not sure, but these styles have different logic for how they're rendered, and I was just maintaining that. I really just wanted to propose an alternate organizational approach to Brooke's PR
Comment on lines
+26
to
+33
| const State = { | ||
| Default: 'default', | ||
| Hover: 'hover', | ||
| Highlight: 'highlight', | ||
| Disabled: 'disabled', | ||
| Checked: 'checked', | ||
| Destructive: 'destructive', | ||
| } as const; |
There was a problem hiding this comment.
What's the purpose of this type? It doesn't look like we're exposing it to consumers, and the fact that the states in this type aren't actually mutually exclusive is confusing (e.g. an option can be both "hovered" and "checked" at the same time, so what is State in this case?)
There was a problem hiding this comment.
To me there are multiple different "state"-type variables:
type State = "default" | "disabled" | "checked" | "highlight"
type ActionType = "default" | "destructive"
and also isHovered (which can apply to any combination of State & ActionType
There was a problem hiding this comment.
Then, in .style.ts we have
get[Element]StateStyle(context), get[Element]ActionTypeStyle(context) and get[Element]HoverStyle(context)
There was a problem hiding this comment.
You'd also probably also want to simplify XYZContextColors to match
There was a problem hiding this comment.
Broad strokes, I like the pattern of using the emotion css object to toggle different styles rather than putting that logic inside a single function.
I do have concerns about how we're defining the styles though, since we have what's logically 4 inter-dependent layers of state (action type, state, hover, darkMode).
It's not perfect, but the way I styled CalendarCell may be relevant here: https://github.com/mongodb/leafygreen-ui/blob/adam/date-picker-lg-3152/packages/date-picker/src/shared/components/Calendar/CalendarCell/CalendarCell.styles.ts
spark33
requested review from
shaneeza,
DesignerDave and
bruugey
as code owners
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.
✍️ Proposed changes
This PR is a set of proposed edits to #2150.
Several things to note -
renderedContext === "form" && actionType === "destructive". I've restricted this from breaking the component and updated the LiveExample to still be interactive with both renderedContext values.overrideHoverBgColorwere identical to the values defined inthemes.ts, so there wasn't really an override going on? If that just needed new values, we can change them, but I've removed them for now.