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

Improve tabset corners; rework border-radius across styles #2077

Merged
merged 5 commits into from
Jan 28, 2025

Conversation

DavidOliver
Copy link
Contributor

@DavidOliver DavidOliver commented Jan 27, 2025

One reason for this suggestion is avoiding the optical mismatch between inner and outer frames:

image

This introduces an inner radius and an outer radius:

image

Summary:

  • Most elements' border radii are reduced to 3px; nearer to what they were before, but still very slightly rounder if I'm not mistaken, and so not such a stark change.
  • Tabset frames' border radius is increased to 12px.
  • Admonitions being set to the 'inner', smaller radius is the biggest difference to the last release; here they are more square, despite, of course, not always appearing within another frame. I don't mind this, but thought it worth flagging.

Screenshot from 2025-01-27 23 04 23

Screenshot from 2025-01-27 23 04 05

This also introduces the inner/small radius to callback/function headings:

Draft status as there are likely more outers to change to inners. I'll wait to hear whether or not this approach is desired.

@DavidOliver DavidOliver requested a review from josevalim January 27, 2025 22:21
Copy link

github-actions bot commented Jan 27, 2025

@josevalim
Copy link
Member

I'd propose for admonitions to be either:

  1. outer radius, as that's how they will appear most times
  2. being inner conditionally, only if they are inside another frame?

Regarding functions, I don't think they should have radius. The border with radius don't work well IMO. :)

@DavidOliver
Copy link
Contributor Author

DavidOliver commented Jan 27, 2025

I'd propose for admonitions to be either:

  1. outer radius, as that's how they will appear most times
  2. being inner conditionally, only if they are inside another frame?

Maybe we make them 'outer' when not nested, and something between 'outer' and 'inner' when they are inner? This might be a reasonable way of maintaining their current feel while still moving them toward optical radius consistency when they're inner. I'll give that a go and post some screenshots.

Regarding functions, I don't think they should have radius. The border with radius don't work well IMO. :)

Okay, I'll revert.

Thanks.

@DavidOliver
Copy link
Contributor Author

DavidOliver commented Jan 28, 2025

After a little experimentation, it seems to me that a good approach would be to simply use an in-between radius for all admonitions. (Currently 8px.) This allows all admonition boxes, regardless of context, to maintain a more rounded feel relative to the code boxes, which I think helps differentiate them. (Not everything has to be the same.) Deviation of border radius for admonitions due to being in a tabset would likely feel slightly jarring as their coloured top bars would emphasise the difference.

Code boxes are still 3px, which is just rounded enough to nicely soften the corners.

Tabset boxes are 14px, for the "concentric" consideration.

image

image

Callback/function headings border radius has been removed.


I've compared with how it is in main again (all with the same radius), and I feel that having three radii used consistently, with a view to the concentric effect of the corners, gives a better feel on balance.

And I've realised that I like the code boxes having a far more modest radius than the admonition boxes, which somehow feels a little stronger/refined(?). It's not that it's objectively good or bad either way in this respect - just that one pro of this approach is greater differentiation between the two different kinds of elements.

@josevalim
Copy link
Member

Sounds good to me!

@DavidOliver
Copy link
Contributor Author

Do you like the way using the three radii looks? If so, and you're happy to proceed, I'll check everywhere else border radius is used and select the most appropriate of the three custom properties.

@josevalim
Copy link
Member

Yes, if you are happy, I am happy! Feel free to merge and please let me know once you think we are ready for the final release. :)

@DavidOliver
Copy link
Contributor Author

I believe all border radiuses are now properly set, given the new approach, but it would be great if you get a chance to have a quick test, too. When choosing one of the three options for each element, I've tended to match/choose the closest to the last release's radius values.

I'm now doubting the use of inner, middle and outer for custom property names as there's only one place this scheme is actually used in that way - in the tabsets. Shall I rename to sm, md and lg or similar?

@DavidOliver DavidOliver marked this pull request as ready for review January 28, 2025 15:52
@josevalim
Copy link
Member

Shall I rename to sm, md and lg or similar?

Sounds good to me!

@josevalim
Copy link
Member

I went over the changes and they look good to me. Honestly, I don't have a strong opinion on this matter, as long as we have a proper mental model to move forward, I am happy. :)

@DavidOliver
Copy link
Contributor Author

Okay, thanks. For simplicity for others who may make edits without seeing the reason for the naming scheme, I'll convert to plain size names.

@DavidOliver DavidOliver changed the title Use two border radii, inner and outer Improve tabset corners; rework border-radius across styles Jan 28, 2025
@DavidOliver DavidOliver merged commit d8568d4 into elixir-lang:main Jan 28, 2025
5 checks passed
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.

2 participants