-
Notifications
You must be signed in to change notification settings - Fork 487
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
fix(@clayui/css): Mixins clay-select-variant
update to use `clay-cs…
#4047
Conversation
…s` pattern, also deprecated keys should win over new keys fix(@clayui/css): Update Sass maps that use `clay-select-variant` to use new keys: `$date-picker-nav-select` and `$input-select`
…nt` to use new keys
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.
Hey @pat270 I'm taking a look at this to understand more about how everything is working around Clay CSS, so I'm going to ask more questions from now on, probably always doubts.
$date-picker-nav-select: () !default; | ||
$date-picker-nav-select: map-deep-merge((), $date-picker-nav-select); |
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 a question about the unrelated this change:
- Double declaration in sequence: the last declaration rewrite the last declaration, right? why double declaration?
- Once you removed the last declaration, what's the expected side effect? if it exist, of course.
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.
@matuzalemsteles Not a problem, map-merge
takes whatever was declared in the first map and combines the two. If there are two keys with the same name in both maps, whatever is declared in the second map wins between the two. This is a way (hack?) to provide default values with Sass maps.
An example that may help explain better:
// This one replaces `$map: () !default;` below it
$map: (
background-color: #eee,
color: red,
) !default;
$map: () !default;
$map: map-merge((
color: black,
font-size: 16px,
), $map);
// $map will be:
$map: (
background-color: #eee,
color: red,
font-size: 16px,
);
If we don't use map-merge
while providing default values:
$map: (
background-color: #eee,
color: red,
) !default;
$map: (
color: black,
font-size: 16px,
) !default;
// $map will be:
$map: (
background-color: #eee,
color: red,
);
The reason we don't need it here is because we aren't providing any default values for $date-picker-nav-select
.
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.
Hmm makes sense, thanks for the clarification.
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
LGTM and merge! |
I'm not sure why it was for 13 days here... |
…s` pattern, also deprecated keys should win over new keys
fix(@clayui/css): Update Sass maps that use
clay-select-variant
to use new keys:$date-picker-nav-select
and$input-select
fixes #4046