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

Convert notes to Remark admonitions #725

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Convert notes to Remark admonitions #725

wants to merge 1 commit into from

Conversation

nickserv
Copy link
Member

@nickserv nickserv commented Dec 21, 2020

Suggested in #701 (comment)

I tried to choose the admonition types based on the existing note content but I'd appreciate feedback on which types to use.

@nickserv
Copy link
Member Author

Ugh, looks like the commit hook broke the formatting on some of these.

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

The color contrast in light mode is really bad in some cases. For caution and info we have merely 1.95 and 1.71. Not that these numbers are accurate representations of the perceived contrast but these support how I perceive them: barely readable.

Regarding the implementation:
Why do we need this special syntax? Would wrapping them in a div with appropriate CSS classes not suffice?

@MatanBobi
Copy link
Member

MatanBobi commented Dec 21, 2020

Ugh, looks like the commit hook broke the formatting on some of these.

Sorry, still didn't get the chance to fix that :/

The color contrast in light mode is really bad in some cases. For caution and info we have merely 1.95 and 1.71. Not that these numbers are accurate representations of the perceived contrast but these support how I perceive them: barely readable.

Regarding the implementation:
Why do we need this special syntax? Would wrapping them in a div with appropriate CSS classes not suffice?

IMO, I see no reason to invent something of our own when there's already an implementation for it within docusaurus.. If we do have some problems with the colors, maybe we can see if there's a way to enhance those (I'm pretty sure there is through the theming).

@MatanBobi
Copy link
Member

@nickmccurdy did the formatting break anything? I only see that it removed some parens..

@alexkrolick
Copy link
Collaborator

alexkrolick commented Dec 22, 2020

The emoji keys are kind of nice, but these colors are pretty extreme... I would say they fall into an area where black text is preferable although the contrast ratio can be a bit misleading when it comes to perception of these types of bright colors. I'd probably tone it down as @eps1lon suggested. Something like the good ole' Bootstrap Alert backgrounds should work: https://getbootstrap.com/docs/5.0/components/alerts/

Screen Shot 2020-12-21 at 5 18 41 PM


Screen Shot 2020-12-21 at 5 07 09 PM

Screen Shot 2020-12-21 at 5 06 38 PM

@nickserv
Copy link
Member Author

I didn't choose the colors, this is the stock theme for Docusaurus 2. I agree that the accessibility is an issue here, but having a visual distinction between different types of admonitions is still useful, and we can customize the theme for better accessibility. Alternatively we could use the Docusaurus 1 theme which seems to have more muted background colors with better contrast. https://github.com/elviswolcott/remark-admonitions#classic-docusaurus-v1

@MatanBobi
Copy link
Member

I didn't choose the colors, this is the stock theme for Docusaurus 2. I agree that the accessibility is an issue here, but having a visual distinction between different types of admonitions is still useful, and we can customize the theme for better accessibility. Alternatively we could use the Docusaurus 1 theme which seems to have more muted background colors with better contrast. https://github.com/elviswolcott/remark-admonitions#classic-docusaurus-v1

I tried using that theme but it looks odd on light might, I can create a PR with that so everyone will see :)

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.

4 participants