Conversation
app/components/card_component.rb
Outdated
| case style | ||
| when :primary | ||
| "border-zinc-300 border rounded-md p-4 space-y-4" | ||
| when :grey |
There was a problem hiding this comment.
I haven't taken a look at this while change yet but wanted to give some quick feedback since I know code reviews are moving quickly! We should not put any color names or visual details (like "white" and "gray" in this PR) as parameters to the components. This is because once the components are themed, those variants will very likely no longer be the color they were in our wireframe theme, and the parameters won't make sense. Please rename all parameter values to be semantically meaningful, checking in with design if you need to. "primary"/"secondary" are a precedent we've used elsewhere, but if this represents some other distinction that's fine too, as long as it's semantic.
There was a problem hiding this comment.
Yup that makes sense!! thank you for that feed back!
mrotondo
left a comment
mrotondo
left a comment
There was a problem hiding this comment.
One small suggestion that might clarify some naming, but looks good!
app/components/reveal_component.rb
Outdated
| def initialize(summary_text:, icon: nil, style: :primary) | ||
| @summary_text = summary_text | ||
| @icon = icon | ||
| @style = |
There was a problem hiding this comment.
[dust] This is a little funky if you don't know the design context. Maybe consider renaming this instance variable to @card_style to clarify a bit?
2 participants
A/C from Design:
Grey is the primary color for the reveal
White is the primary color for the card
Adds grey variant to reveal and card component
The colors are actually set on the card, so the @Style in the reveal is being set to what the card components needs to be the correct color.