Rework alert shortcode

[chalin]

• This is a proposed rework of the alert.html@02df04c0f2 shortcode.
• Contributes to
  • #906
  • #939

This doesn't offer as clean a solution as I would have liked, but I feel that it is a step in the right direction. Note that this will introduce a breaking change.

I'll continue to explore if/how this shortcode can further be simplified -- ideally, so that we can avoid the use of the markdownify shortcode argument -- but any further changes will be submitted through another PR.

Preview links:

• https://deploy-preview-941--docsydocs.netlify.app/docs/deployment/#deployment-with-netlify
• https://deploy-preview-941--docsydocs.netlify.app/docs/adding-content/shortcodes/#blockscover

[deining]

I'll continue to explore if/how this shortcode can further be simplified -- ideally, so that we can avoid the use of the markdownify shortcode argument

I rebased your PR to HEAD of repo and added a second commit, that exactly does what you wished/proposed: discarding/avoiding the shortcode argument markdownify.

This topic is a bit tricky: the key to the solution is the common mark spec:

An HTML block is a group of lines that is treated as raw HTML
...
It ends with the first subsequent line that meets a matching end condition

We do introduce html blocks with our shortcode, so if we want to get our markup evaluated, we have to make sure that within our shortcode, an end condition for the introduced html block is met again. The easiest way to fulfill the end condition is to insert a blank line (number 7 in the spec), and that's exactly what my second commit does.

The irony is that the solution proposed with your first commit worked quite well already (were you aware of that?). In the end my second commit is only a reinforcement of your original approach in the sense that it now allows even corner cases like:

{{% alert title="Note" color="info" %}}**Bold** content{{% /alert %}}

that weren't covered with your first approach. Everything else did work already (even without shortcode argument markdownify!).

but any further changes will be submitted through another PR.

I hope my change is small enough to justify adding a second commit here.

I now wonder whether you still considering this a breaking change? I hope we can finalize this issue soon, I don't think we can do any better here.

[fekete-robert]

Hi, since you are already working on this shortcode, I'd suggest getting rid of the h4 heading, and replacing it with a non-heading styling. Currently such alerts can appear in the page-level toc, and they also break the heading structure (for example, a h4 heading from the alert appears under a h2 heading), which is against best practices.

[deining]

I'd suggest getting rid of the h4 heading, and replacing it with a non-heading styling.

Great suggestion, done.

@fekete-robert: can you comment on/review this PR?

[fekete-robert]

I'm swamped by work right now, but I hope to take a look during the weekend

[chalin]

Thanks for the updates @deining. I've revisited this PR, making the following adjustments:

• Recovered the .h4 styling of the alert header, without making it heading element
• Changed the shortcode file suffix to .md

Note that I still can't get the alert shortcode to play well when used in lists, for example. I'm unsure if this is a Hugo bug/feature or due to the shortcode itself. I don't have time to investigate this further now, so I've opened the following issue to track the problem:

• #1672

PTAL @deining @fekete-robert. Note that I'm OOO next week so feel free to merge this if you judge that it's ready.

[chalin]

Hmm, given that this is potentially a breaking change, it should be postponed until:

• #1667

I'll update the CHANGELOG.

/cc @geriom

[deining]

Thanks for the updates @deining. I've revisited this PR, making the following adjustments:

* Recovered the `.h4` _styling_ of the alert header, without making it heading element

That's great, I was aware of this defect already, your solution is great!

* Changed the shortcode file suffix to `.md`

Fine for me.

Note that I still can't get the alert shortcode to play well when used in lists, for example.

I got aware of this defect recently, too.

I'm unsure if this is a Hugo bug/feature or due to the shortcode itself.

This is a bug in the shortcode itself. Whitespace handling inside the shortcode is really trick. I addressed this issue in an additional commit which hopefully resolves this problem.

I don't have time to investigate this further now, so I've opened the following issue to track the problem:

* [Alert shortcode in markdown not playing nice when indented #1672](https://github.com/google/docsy/issues/1672)

This PR hopefully closes #1672.

PTAL @deining @fekete-robert. Note that I'm OOO next week so feel free to merge this if you judge that it's ready.

Yes, I think it's ready for merging now, but from my understanding, this should go into the v0.8 release, correct? What is the time schedule date for this release?

[deining]

Rebased PR and fixed code style issues. IMHO this PR is eventually ready for merging.

[deining]

I move breaking change note to the correct section in the CHANGELOG. With two approving reviews given, any reason to not include this in the upcoming 0.10.0 release?

[chalin]

I move breaking change note to the correct section in the CHANGELOG.

Thanks.

With two approving reviews given, any reason to not include this in the upcoming 0.10.0 release?

When I last tested this, it unexpectedly broke some use cases on the OTel website. I investigated, and couldn't figure out why it wasn't working. It occurred to me that it might be a bug in Hugo. I haven't had the time to investigate further nor create a small enough repro, and so let this slide until a later release.

[deining]

I move breaking change note to the correct section in the CHANGELOG.

Thanks.

You are welcome.

With two approving reviews given, any reason to not include this in the upcoming 0.10.0 release?

When I last tested this, it unexpectedly broke some use cases on the OTel website.

Yes, I remember you raised an according issue.

I investigated, and couldn't figure out why it wasn't working. It occurred to me that it might be a bug in Hugo.

As mentioned in my reply, I investigated this and it turned out it was a whitespace issue in the shortcode, which was fixed meanwhile with commit 4da2600a504. I'm quite sure this is not an Hugo issue.

I haven't had the time to investigate further nor create a small enough repro, and so let this slide until a later release.

Can you please test the latest version of this shortcode on the OTel website again? I'm pretty confident it won't cause any problems any more!