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

doc: add 'not recommended' blockquotes #52814

Closed
wants to merge 1 commit into from

Conversation

RedYetiDev
Copy link
Member

@RedYetiDev RedYetiDev commented May 3, 2024

Fixes: #52743

This PR adds the ability to add a warning blockquote via the following syntax:

> Warning: ...

Which will be rendered as:
image

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/nodejs-website
  • @nodejs/web-infra

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels May 3, 2024
@jasnell
Copy link
Member

jasnell commented May 4, 2024

While I'm fine with the idea, the white text on the red background is difficult for my eyes to read.

@RedYetiDev
Copy link
Member Author

While I'm fine with the idea, the white text on the red background is difficult for my eyes to read.

Good to note! What color scheme would you recommend?

@jasnell
Copy link
Member

jasnell commented May 4, 2024

I honestly don't know what to recommend :-/ ...

@RedYetiDev
Copy link
Member Author

I honestly don't know what to recommend :-/ ...

I'll experiment. Right now it's the same as Stability: 1 blockquotes.

@RedYetiDev
Copy link
Member Author

A border is also possible:

image

@jasnell
Copy link
Member

jasnell commented May 4, 2024

That's a LOT more readable to my eyes.

@RedYetiDev RedYetiDev added the review wanted PRs that need reviews. label May 6, 2024
@RedYetiDev
Copy link
Member Author

(Request Review) @nodejs/documentation

tools/doc/html.mjs Outdated Show resolved Hide resolved
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

I assume I'm a bit out of the loop here, but why are we not sticking with existing concepts such as <strong class="critical"> and maybe restyle that a bit? And if you really want to introduce a custom syntax for blockquotes, why not adopt GitHub's mechanism?

@RedYetiDev RedYetiDev removed the review wanted PRs that need reviews. label May 9, 2024
@RedYetiDev
Copy link
Member Author

I assume I'm a bit out of the loop here, but why are we not sticking with existing concepts such as <strong class="critical"> and maybe restyle that a bit? And if you really want to introduce a custom syntax for blockquotes, why not adopt GitHub's mechanism?

I decided to go with the same syntax that the Stability: ... blockquote use. Instead of using the default critical error (because they can be [a] hard to read, and [b] these aren't critical), I went with a red box.

Copy link
Contributor

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

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

UI/UX is LGTM in my opinion !!

Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

I feel that the styling is a bit over-the-top for the "severity" of these "warnings", which I'd say are more recommendations than actual "warnings".

To be honest, the idea behind is legitimate, but I'm not sure about adoption + how useful this would be for the reader. I do believe some distinction could bring benefits, but not sure what sort of change is warranted here.

Not to mention you're technically changing the spec we have for our docs but not documenting it anywhere. We have a guide for the API docs, you should update that guide too.

Lastly, unrelated, and this PR can land regardless, but, we're about to start a revamp on the tooling behind the API docs, so this code will be discarded; If this PR lands I'll create a counterpart code to handle this, but I believe we should handle this differently.

I'm extremely against using magic "string searches" with prefixes, such as a string that starts with "Warning:" to then apply styling on it. It's a bad design pattern, and should be avoided. I strongly agree with @tniessen here; We could simply surround these with HTML elements that represent such "quotes" or do it as GitHub does with the symbols in the beginning...

@RedYetiDev
Copy link
Member Author

True, your right that the the redesign will incorporate a lot of changes, so I'll close this, but I recommend something similar be included in the redesign

@RedYetiDev RedYetiDev closed this May 10, 2024
@ovflowd
Copy link
Member

ovflowd commented May 10, 2024

True, your right that the the redesign will incorporate a lot of changes, so I'll close this, but I recommend something similar be included in the redesign

Feel free to go over the Figmas and check if we have something similar like this already, otherwise, feel free to make suggestions :)

@RedYetiDev
Copy link
Member Author

True, your right that the the redesign will incorporate a lot of changes, so I'll close this, but I recommend something similar be included in the redesign

Feel free to go over the Figmas and check if we have something similar like this already, otherwise, feel free to make suggestions :)

Oh! I didn't know we had figmas, I'll take a look!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(doc): Add warning blocks to API docs
6 participants