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

Tone down admonition coloring #2499

Merged
merged 1 commit into from May 19, 2024

Conversation

fredrikekre
Copy link
Member

This patch tones down the admonitions by removing the background color in the admonition header and the lighter color in the body. Instead the admonition header text is colored.

@fredrikekre fredrikekre force-pushed the fe/scratch-some-paint-from-admonitions branch 2 times, most recently from c17886f to 1ffec1e Compare May 11, 2024 15:39
@fredrikekre fredrikekre requested a review from mortenpi May 11, 2024 15:39
@fredrikekre fredrikekre marked this pull request as ready for review May 11, 2024 15:39
@fredrikekre fredrikekre force-pushed the fe/scratch-some-paint-from-admonitions branch from 1ffec1e to 13a92ea Compare May 11, 2024 15:40
@fredrikekre
Copy link
Member Author

Increased the border thickness a bit too since otherwise it was a bit difficult to see the color of the borders. I am happy with this now.

@mortenpi
Copy link
Member

mortenpi commented May 12, 2024

LGTM! Yea, I think this will make them a bit easier to use, especially if you want to use more than one. The current ones a very eye-catching.

For consistency, should we increase the thickness of the docstrings blocks and pre blocks maybe?

image

@fredrikekre
Copy link
Member Author

Maybe? And also round the corners like admonitions then?

@mortenpi
Copy link
Member

mortenpi commented May 12, 2024

And also round the corners like admonitions then?

Or make admonitions rectangles? 😅

Actually, scratch that. I tested it a bit, and I actually like it better when docstrings and code blocks are rounded. So if you feel like it's worth making the change, let's go with that?

image

image

@fredrikekre fredrikekre force-pushed the fe/scratch-some-paint-from-admonitions branch from 13a92ea to 06f9ad1 Compare May 13, 2024 08:30
@fredrikekre
Copy link
Member Author

Updated, and also the border-bottom of docstring headers.

@fredrikekre
Copy link
Member Author

and also the border-bottom of docstring headers.

Maybe this isn't needed actually?

@mortenpi
Copy link
Member

and also the border-bottom of docstring headers.

Maybe this isn't needed actually?

Do you mean getting rid of the border altogether or keeping it 1px? I think my mild preference is to just keep that 1px.

image

image

image

@fredrikekre
Copy link
Member Author

Do you mean getting rid of the border altogether or keeping it 1px?

I mean that increasing it isn't needed and agree that 1px looks nicer.

This patch tones down the admonitions by removing the background color
in the admonition header and the lighter background color in the body.
Instead the text in the admonition header is colored, and the colored
border thickness increased slightly.

The border width of code blocks and docstrings have been increased to
match the new border width of admonitions. Code blocks and docstrings
now also have a (non-zero) border radius to match admonitions (which
already were rounded before this patch).
@fredrikekre fredrikekre force-pushed the fe/scratch-some-paint-from-admonitions branch from 06f9ad1 to b763bfe Compare May 14, 2024 05:51
@fredrikekre fredrikekre requested review from mortenpi and removed request for mortenpi May 17, 2024 22:31
Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

LGTM! I don't think we have anything else coming, so shall we tag 1.5.0 right away?

@fredrikekre fredrikekre merged commit bc5447c into master May 19, 2024
23 checks passed
@fredrikekre fredrikekre deleted the fe/scratch-some-paint-from-admonitions branch May 19, 2024 07:19
@fredrikekre
Copy link
Member Author

I could finish up #2496 before, but doesn't really matter.

@mortenpi
Copy link
Member

Happy to hold off until then. The two changes go together nicely.

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

Successfully merging this pull request may close these issues.

None yet

2 participants