-
Notifications
You must be signed in to change notification settings - Fork 972
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
Allow inline summary cutoff #2581
base: next
Are you sure you want to change the base?
Conversation
Honestly I'm not too sure about that. The original intent when it was added was to put it on an empty line and putting it in the middle of some HTML - I don't know... |
That's extremely fair. In my case, I don't use it very often, but the current case where I do use it is for cutting sentences off as a kind of cliffhanger. Things like "So, I took a look at X, and <cutoff>". I just made a complete version that accounts for all kinds of markdown syntax because if I was going to do anything, I might as well do it right instead of just adding a And like, I guess that there might be cases where you might want to cut off in the middle of a blockquote or something, although those are probably much rarer. If you do want it to remain on its own line, I can remove the summary-specific stuff and just keep it to fixing shortcodes for now. I do think that |
Not sure about the technical details of this PR (I haven't looked into the code), but I found that even |
Short answer, yes, Long answer, yes but it will change the code so that it actually emits the end of the paragraph and leave a cutoff marker in its place, so, you will need to modify your templates if they were manually adding the paragraph end marker back (alongside an ellipsis) in some capacity. |
This could potentially be a built-in template the user can override like The issue with putting the ellipsis in via CSS is that some user agents (Atom/RSS feed readers for example) might not load the stylesheet at all and then the ellipsis is gone entirely. I'm also not sure what implications would it have for accessibility. |
Actually, yeah, that seems the most reasonable. Could also provide the template with the contents of the summary so you could, for example, only emit an ellipsis if there isn't punctuation at the end of the line. I'll make that the default. |
c3ac709
to
de5b94c
Compare
Marking this as draft since I'm moving the shortcode fixes to a separate PR. Will rebase on top of that once it merges. |
de5b94c
to
7ecf8c7
Compare
4db3353
to
ef3ced0
Compare
Rebased this on the other change and added a template version of the summary cutoff. By default, it ends the summary in an ellipsis if it was cut off inline. |
7cf301b
to
45099d5
Compare
45099d5
to
f7b9d32
Compare
@@ -793,15 +796,50 @@ pub fn markdown_to_html( | |||
.position(|e| matches!(e, Event::Html(CowStr::Borrowed(CONTINUE_READING)))) | |||
.unwrap_or(events.len()); | |||
|
|||
// determine closing tags missing from summary |
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.
Could we error if the summary marker is inside a tag? Eg <i <-- more --> class='' />
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.
Since that's just straight-up invalid HTML, I'd be more inclined to just make the <!-- more -->
regexp only match if it's by itself (i.e. not nested inside something like that). Will add a test for it.
Personally, I think that an error would be weird; we don't emit errors for invalid HTML otherwise, so, it's better to just ignore it and treat it like otherwise malformed HTML.
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.
Fair enough
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.
So, I tried adding a test for this, but the markdown parser just isn't happy with it. If it's valid HTML, it parses, and if it's not, it just escapes it. So, in your example, it shows up as <i
at the end of the summary.
I'm inclined to just say that's okay, honestly. We could detect if it's inside any custom HTML tags, like <i><!-- MORE --></i>
, but that goes against the principle that we aren't accounting for custom HTML tags at all and those just remain opened.
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.
Sounds good
&None, | ||
) | ||
.context("Failed to render summary cutoff template")?; | ||
summary_html.push_str(&summary_cutoff); |
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.
Do we want a template or a string in the config? Template is more powerful but might be overkill?
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 feel like the template is ideal, although it's unlikely many people will change the template. The original idea was proposed as an easy option since even anchor-link.html
has a simple template associated with it.
Without a template, you can't choose whether to emit an ellipsis based upon the closing punctuation, so, I'm inclined to say that the template is strictly better.
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.
One nit but good otherwise
@@ -0,0 +1 @@ | |||
{% if summary is matching("\PP$") %}…{% endif %} |
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.
Can we default to an ellipsis without the test and mention that in the docs as an example?
matching
will currently create a new regex everytime this template is called so it's not ideal perf wise as a default
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.
Oh, I had no idea that was the case; I'm fine with that, then.
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.
FWIW, no idea where Tera 2 is currently, but I'd be willing to help figure out how to solve this problem there. Feels like it should be doable to let the functions know about the constant arguments and optimize themselves accordingly.
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.
It's possible to fix in tera 1 I think yes, just have a cache for the compiled regexes
f7b9d32
to
ca4ea8e
Compare
next
branch?This worked previously, but was broken by a recent change. Essentially, because the
InlineHtml
event is not considered for the<!-- more -->
marker, it has no meaning at the moment when not placed on its own line.This alters the code to allow this again, but does so in a more reasonable way:
summary-cutoff.html
template at the end of the summary if this happens, which acts depending on the summary before the cutoff and the current language. It defaults to showing an ellipsis if the summary doesn't end in punctuation.I've also added tests to verify this behaviour works.
Fixes #2562.