Skip to content
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(Card): use common constants for card's radiuses #1682

Closed
wants to merge 2 commits into from

Conversation

Arucard89
Copy link
Contributor

@Arucard89 Arucard89 commented Jun 28, 2024

@gravity-ui-bot
Copy link
Contributor

Preview is ready.

@gravity-ui-bot
Copy link
Contributor

Visual Tests Report is ready.

@Arucard89 Arucard89 marked this pull request as ready for review June 28, 2024 11:21
@@ -157,11 +157,11 @@ $block: '.#{variables.$ns}card';

&_size {
&_m {
--_--border-radius: 8px;
--_--border-radius: var(--g-border-radius-l);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we use --g-border-radius-l for size_m?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Card's border sizes don't have straight accordance to radius constants. I have choosen constants by values.
https://github.com/gravity-ui/uikit/blob/main/styles/themes/common/_index.scss#L13

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amje I think it is a not good idea to use symantic variables in such cases

Copy link
Contributor Author

@Arucard89 Arucard89 Jun 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about adding variables for cards to _index.scss?
For example:
—g-border-radius-card-m
—g-border-radius-card-l

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about adding variables for cards to _index.scss?
For example:
—g-border-radius-card-m
—g-border-radius-card-l

@korvin89 , @amje, I mean like this: a50b460

In my opinion, these variables also helps to make themisation easier, without using CSS API

@Arucard89 Arucard89 closed this Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants