-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(defaultProps): replace theme variables with classes #3701
Conversation
828aa40
to
0534562
Compare
93969bf
to
194fd37
Compare
952725b
to
3919e6a
Compare
3919e6a
to
3c05743
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 🙏
I just identified some things while reviewing.
Also, I noticed some components have two distinct areas:
- (1) one where the props are defined and used by
useDefaultProps
:
components: { "HvCard": { (...) }, (...) }
- (2) and one where we have some more styling and/or variants defined (and these values are mapped out to CSS vars):
card: {
titleVariant: "label",
subheaderVariant: "caption1",
subheaderColor: theme.colors.secondary,
}
I believe there are still some variables in (2) that could be mapped out to (1). I identified those in my review. Also, when necessary, I also think we should map out the variables related to CSS styling by creating new classes. For example, regarding the card
component, we could create a new class for the subheaderColor
to remove it from (2). The only values related to CSS styling I see remaining in (2) are the header
height and second level height because these seem like variables users would want to access.
Also, having variants as CSS vars is something I believe we should try to remove in this PR since these are not valid values (ex: titleVariant
and subheaderVariant
for the card component). Thus, I think we should review our mapping to omit them.
What of you think?
15b81e1
to
246aec6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What a huge effort 😅 💪
Looking great overall. I still want to make a deeper review, but I'll leave these comments for now:
- I think we should separate the component improvements & missing
useDefaultProps
to a separate PR, to reduce the scope and size of this PR - It would be nice if we could extract more common color keys like we did for
containerBackgroundHover
I agreed with @MEsteves22 suggestion and tried removing the rest of the variables. The theme variables left are mostly button variants, colors and a few other values that weren't clear to me how to substitute, maybe we can deal with those types individually in another issue, or if you have a suggestion that I'm not seeing say so. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌🏻
Just noticed two small things:
.../components/ScrollTo/Horizontal/HorizontalScrollListItem/HorizontalScrollListItem.styles.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🙏
We'll need to review the "White Labelling" page because a lot of the variables used there are no longer available. But I think we can do that in another PR.
As future work I also think that we should try to remove variants as CSS vars since these are not valid values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome 👌
I have replaced most of the theme component variables with classes to the
defaultProps
.Some values I wasn't able to remove, these were mostly values used with the
activeTheme
( mostly typography variants ).Other values I left because they seemed useful to customize the component and provided some abstraction, these were mostly values that are used throughout the component in different classes or sub components.
See if you agree with the ones I left out or the ones I left in.