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

Refactor border-radius styles to use CSS variable for consistency #2023

Merged
merged 4 commits into from
Jan 17, 2025

Conversation

hichemfantar
Copy link
Contributor

@hichemfantar hichemfantar commented Jan 17, 2025

I noticed border radius inconsistencies so i addressed them via a single variable.
Update border-radius styles across various components to utilize a CSS variable, ensuring consistency in design. Adjust the variable value for improved appearance.

image

image

image

Copy link

github-actions bot commented Jan 17, 2025

@hichemfantar hichemfantar marked this pull request as ready for review January 17, 2025 11:42
@josevalim
Copy link
Member

Thank you! I like the consistency but I think the inline styles, such as code, should have a --inlineBorderRadius or similar, and be indeed smaller. Probably keeping it at 2px. Can you please update it?

@hichemfantar
Copy link
Contributor Author

are you saying codeblocks like this one should have a smaller radius?

image

@hichemfantar
Copy link
Contributor Author

oh my bad i understand now i'll make the change

Comment on lines 17 to 18
background-color: transparent;
backdrop-filter: blur(2px);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
background-color: transparent;
backdrop-filter: blur(2px);
background-color: var(--codeBackground);

I tried it in a couple places but I found the blurred colors confusing. I did like the border though, can we revert it partially?

Copy link
Contributor Author

@hichemfantar hichemfantar Jan 17, 2025

Choose a reason for hiding this comment

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

understandable, i increased the contrast. if you still find it confusing i'll revert the change

before
image

after
image

@josevalim
Copy link
Member

To be clear, I meant the one for mix help docs should stay with 2px: example

@hichemfantar
Copy link
Contributor Author

hichemfantar commented Jan 17, 2025

i find 4px to be well balanced and closer to the theme, what do you think?

2px
image

4px
image

@hichemfantar
Copy link
Contributor Author

@josevalim another example zoomed out

2px
image

4px
image

@josevalim josevalim merged commit be954bd into elixir-lang:main Jan 17, 2025
4 checks passed
@josevalim
Copy link
Member

josevalim commented Jan 17, 2025

💚 💙 💜 💛 ❤️

@hichemfantar hichemfantar deleted the better-border-radius branch January 17, 2025 13:40
@DavidOliver
Copy link
Contributor

@hichemfantar, @josevalim, did you mean for the tabs to be affected?

image

image

Although the tabs are now more visually distinguished from each other, the flatter look feels neater to me.

@hichemfantar
Copy link
Contributor Author

looks better with a gap, i'll push a fix

image

@hichemfantar
Copy link
Contributor Author

#2027

@DavidOliver
Copy link
Contributor

Do you prefer the new corner style?

Separately to preferring the visual feel of how it was, a (minor) concern I have is that on narrow/mobile screens this approach is more likely to see the tabs taking up more space than is available. Would you be averse to reverting the tabs style?

But if you and @josevalim are happy, not to worry.

@hichemfantar
Copy link
Contributor Author

I prefer the one with spacing because it gives the elements room to breathe and is more aligned with the theme thanks to the rounded corners.
Also the original design was also broken despite the small radius because you could see the rounded corners clashing with each other due to the lack of spacing

@hichemfantar
Copy link
Contributor Author

hichemfantar commented Jan 17, 2025

Wrote a new a implementation that's pretty much better that all the ones we discussed
Pretty clean right? opening a pr soon

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants