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

Update admonitions, making title optional #3389

Merged
merged 5 commits into from
Mar 30, 2022
Merged

Conversation

julieg18
Copy link
Contributor

@julieg18 julieg18 commented Mar 24, 2022

  • makes title optional, taking it off by default

Code Example:

<admon>

I don't need a title since I'm just a block of text.

</admon>

<admon title="Custom Title" type="tip">

I need a custom title for some reason

</admon>

Result:
image

Fixes #3381

@shcheklein shcheklein temporarily deployed to dvc-org-update-admoniti-krayu1 March 24, 2022 16:32 Inactive
@julieg18 julieg18 self-assigned this Mar 24, 2022
@julieg18 julieg18 marked this pull request as draft March 24, 2022 16:41
content/docs/index.md Outdated Show resolved Hide resolved
@julieg18 julieg18 requested review from a team March 24, 2022 16:43
Copy link
Contributor

@rogermparent rogermparent left a comment

Choose a reason for hiding this comment

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

Looks nice! I'm not sure if this is exactly what @shcheklein was thinking, but the option for an alternate minimal admonition seems pretty useful. I could see possible concerns around consistency between the two, though.

@julieg18
Copy link
Contributor Author

Looks nice! I'm not sure if this is exactly what @shcheklein was thinking, but the option for an alternate minimal admonition seems pretty useful. I could see possible concerns around consistency between the two, though.

We could take off the title entirely! I wasn't sure which (asked in #3381), but for now, I made it an option 🤔

@julieg18 julieg18 requested a review from jorgeorpinel March 25, 2022 16:36
@daavoo
Copy link
Contributor

daavoo commented Mar 28, 2022

I think the title might be useful on some occasions.

@julieg18 julieg18 temporarily deployed to dvc-org-update-admoniti-krayu1 March 28, 2022 19:14 Inactive
@julieg18 julieg18 temporarily deployed to dvc-org-update-admoniti-krayu1 March 28, 2022 19:20 Inactive
@julieg18 julieg18 marked this pull request as ready for review March 28, 2022 19:21
@jorgeorpinel
Copy link
Contributor

Leaving title optional can't hurt. What I'm not sure we need is the default titles e.g. "Warning" in the description usage example. I think if title is used, a value should be provided, otherwise the syntax looks a bit strange to me.

@jorgeorpinel jorgeorpinel removed their request for review March 29, 2022 01:06
@iesahin
Copy link
Contributor

iesahin commented Mar 29, 2022

Could we make the titles similar to <details> block? Instead of adding it as an attribute, we could update the text itself like:

<admon type="tip">

## Very important tip

This is very important

</admon>

This is for the sake of consistency. I don't see the current way as particularly useless and if this is too much work, we can postpone it.

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Mar 29, 2022

make the titles similar to <details> block?

We're actually trying to make <details> work like <admon> instead @iesahin 😅 . See #3388

@iesahin
Copy link
Contributor

iesahin commented Mar 30, 2022

Ah yeah, I noticed that after writing this. I think it's better to have the titles as first elements as in current details, it makes it easier to change details/admon to a standard section.

For linking there may be a slug or anchor attribute in the tag apart from the title. We can use it for linking. This also relaxes us to come up with very short titles for these sections.

@julieg18
Copy link
Contributor Author

julieg18 commented Mar 30, 2022

Leaving title optional can't hurt. What I'm not sure we need is the default titles e.g. "Warning" in the description usage example. I think if title is used, a value should be provided, otherwise the syntax looks a bit strange to me.

Good point! I'll take off the default title option!

As for using a title attribute vs taking a ### heading from the content, both are easily doable. I'm not sure which one is better... @jorgeorpinel, @iesahin since the original intent of this pr is to fix #3381, should we go ahead and merge this then discuss the title vs ### heading in a separate issue?

@julieg18 julieg18 temporarily deployed to dvc-org-update-admoniti-krayu1 March 30, 2022 14:37 Inactive
@jorgeorpinel
Copy link
Contributor

For linking there may be a slug or anchor attribute
discuss the title vs ### heading in a separate issue

We're discussing that in #3383 , feel free to mention your idea there @iesahin (may be the easiest way indeed).

Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Assuming default title has been removed. Thanks @julieg18

@julieg18 julieg18 merged commit 6187b3b into master Mar 30, 2022
@julieg18 julieg18 deleted the update-admonitions branch March 30, 2022 20:22
iesahin pushed a commit that referenced this pull request Apr 11, 2022
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.

admonitions titles take too much precious space
6 participants