-
Notifications
You must be signed in to change notification settings - Fork 888
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
Rework alert shortcode #941
base: main
Are you sure you want to change the base?
Conversation
b0f82c2
to
2996bd1
Compare
2996bd1
to
e3b96d6
Compare
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 This topic is a bit tricky: the key to the solution is the common mark spec:
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:
that weren't covered with your first approach. Everything else did work already (even without shortcode argument
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. |
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. |
Great suggestion, done. @fekete-robert: can you comment on/review this PR? |
I'm swamped by work right now, but I hope to take a look during the weekend |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to work fine to me, thanks!
53126a1
to
8a1b927
Compare
Thanks for the updates @deining. I've revisited this PR, making the following adjustments:
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: PTAL @deining @fekete-robert. Note that I'm OOO next week so feel free to merge this if you judge that it's ready. |
Hmm, given that this is potentially a breaking change, it should be postponed until: I'll update the CHANGELOG. /cc @geriom |
That's great, I was aware of this defect already, your solution is great!
Fine for me.
I got aware of this defect recently, too.
This is a bug in the shortcode itself. Whitespace handling inside the shortcode is really trick.
This PR hopefully closes #1672.
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this PR is eventually ready for merging 😄.
f45fdaf
to
c722dfc
Compare
d17d346
to
c4574c8
Compare
Rebased PR and fixed code style issues. IMHO this PR is eventually ready for merging. |
74c3122
to
95e2f72
Compare
95e2f72
to
c03f1a3
Compare
I move breaking change note to the correct section in the |
Thanks.
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. |
You are welcome.
Yes, I remember you raised an according issue.
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.
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! |
{{% %}}
#939This 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: