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

Add admonition feature #210

Merged
merged 1 commit into from
Mar 23, 2022
Merged

Add admonition feature #210

merged 1 commit into from
Mar 23, 2022

Conversation

julieg18
Copy link
Contributor

@julieg18 julieg18 commented Mar 23, 2022

Basically a copy of dvc.org#3371

Code Example:

<admon type="warn">

text text text

</admon>

Design:

image

Type options:

Required type prop

  • info (default emoji is ℹ️)
  • tip (default emoji is 💡)
  • warn (default emoji is ⚠️)

Icon options:

Optional icon prop.

tip - 💡
info - ℹ️
warn - ⚠️
fire - 🔥
exclamation - ❗
beetle - 🐞

@julieg18 julieg18 requested a review from a team March 23, 2022 19:39
@shcheklein shcheklein temporarily deployed to cml-dev-add-admonitions-wlp77m March 23, 2022 19:39 Inactive
@julieg18 julieg18 self-assigned this Mar 23, 2022
@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented May 4, 2022

Icon options:
...
exclamation - ❗

Question @iterative/websites : why are there so many icons and which style does each one use? E.g. looks like "exclamation" is type "warn" but with another icon. A bit confusing (minor)

@julieg18
Copy link
Contributor Author

julieg18 commented May 4, 2022

Question @iterative/websites : why are there so many icons and which style does each one use? E.g. looks like "exclamation" is type "warn" but with another icon. A bit confusing (minor)

Essentially there are three type that come with their own color:
info - purple
tip -blue
warn - orange

While these come with default emojis, you can update any type to use a different icon if you'd like by using the icon prop. We originally just had the three emojis, but it was requested that we have extra icon options.

@jorgeorpinel
Copy link
Contributor

Got it. The icon prop info. is what was missing. TBH so far we're not using that feature much. It's unclear to me why we would need alternative icons and those specifically. That said it doesn't hurt to keep it either.

@jorgeorpinel
Copy link
Contributor

No wait... In this PR we used type="exclamation" and it rendered correctly (as a type warn with ! icon). How does that work?

@julieg18
Copy link
Contributor Author

julieg18 commented May 5, 2022

No wait... In this PR we used type="exclamation" and it rendered correctly (as a type warn with ! icon). How does that work?

It actually didn't render as warn! warn is orange, not purple. That's the info type, aka purple, because the admonition defaults to info if it doesn't get a type (or the type doesn't exist).

As for how you still got the exclamation, even though the icon prop wasn't used. If you don't give an icon type, the icon defaults to type. Aka

<admon type='warn'>
  text text text text
</admon>

image

Because the icon name for `warn` is ⚠️.

We probably don't want that behavior though. I'll open a pr that stops icon from defaulting to type so there is less confusion in the future.

@jorgeorpinel
Copy link
Contributor

I see.

If you don't give an icon type, the icon defaults to type

Ok that's the unexpected part indeed. Thanks!

@casperdcl
Copy link
Contributor

​Not sure extra icons not linked to type are useful. Seems to break theme/convention IMO.

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