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: improve admonitions notes #1549

Merged
merged 2 commits into from
Sep 2, 2024

Conversation

bjohansebas
Copy link
Member

Colors have been changed because their meanings were not visually distinguishable, making the documentation difficult to read. Additionally, in dark mode, the colors were not displayed correctly.

Notice:
image
image
image
image
Note:
image
image
image
image
Warn:
image
image
image
image

Copy link

netlify bot commented Jul 28, 2024

Deploy Preview for expressjscom-preview ready!

Name Link
🔨 Latest commit 5e9bc48
🔍 Latest deploy log https://app.netlify.com/sites/expressjscom-preview/deploys/66aad90cae6e9500086192ea
😎 Deploy Preview https://deploy-preview-1549--expressjscom-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@crandmck
Copy link
Member

Thanks @bjohansebas in general, this looks good.

These things are typically called "admonitions" not "asides". Could you change the directory to that just for sake of conforming to industry standards.

Also, this is a fairly sweeping change and I didn't have time to review it in depth yet. Maybe @chrisdel101 or @inigomarquinez can help on that front.

@chrisdel101
Copy link
Contributor

The code changes look fine to me. I didn't go through every single page in preview but the ones I did see look pretty nice!
@bjohansebas If you want to make those directory changes as requested.

@bjohansebas
Copy link
Member Author

@crandmck @chrisdel101 thanks for the recommendation; the directory name change has been made.

@bjohansebas bjohansebas changed the title feat: improve aside notes feat: improve admonitions notes Aug 1, 2024
@crandmck
Copy link
Member

I'll land this, but would like to understand why the check-translation test is failing.

@bjohansebas
Copy link
Member Author

@crandmck It's an issue we've been referencing in #1553. Probably @IamLizu will have the solution and explanation once it's resolved.

@crandmck crandmck merged commit f24789a into expressjs:gh-pages Sep 2, 2024
6 of 7 checks passed
@bjohansebas bjohansebas deleted the improve-notes branch September 2, 2024 20:31
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