Topic List Previews Theme Component

merefield · May 4, 2022, 9:06am

Oh that’s a bug, definitely. We’ll need to investigate that, good catch!

merefield · May 4, 2022, 9:06am

Excellent, thanks for the PR, merged!:

IMPROVE: like icon alignment when no likes by Canapin · Pull Request #16 · merefield/discourse-topic-list-previews-theme (github.com)

Canapin · May 6, 2022, 8:15pm

Is there a way to have a the thumbnail on the category page with latest topics as well?

merefield · May 6, 2022, 9:03pm

Three challenges:

Im fairly sure thumbnails are not serialised here. Feel free to confirm. That could be overcome by enhancing the sidecar plugin.
when I last looked at this the structure of the page did not permit low risk overrides isolated to a leaf template. Generally we don’t want to override the whole page which might cause breaking changes or mask feature updates from core. A PR to core might help here …
Space?

Feel free to submit required PRs

Canapin · May 7, 2022, 12:09pm

Fair points. I might choose another presentation for the category pages, maybe boxed or something like that.

I have another question.
Is it possible to have category pages (which contains topic lists) being displayed in tiles on mobile, but with thumbnails on desktop? I tried several settings tweaks but I couldn’t achieve this.

edit: I achieved it. I’ll detail how I did it a bit later in case it could interest someone else, I added this edit just so nobody spends time explaining me how to do it

So, If I’m not mistaken, here are the settings…

First we need the tiles display only on the mobile view of (at least?) a topic list:

Then the thumbnails for desktop AND mobile:

I was mistaked at first because I thought “tiles” and “thumbnails” were two different presentations of a topic list, both containing the images. but this is not the case. Thumbnails are needed if you want images to appear on a topic list, whether it’s a tile or not.

There is no need to manually add your categories here:

Since we’ll override this setting by enabling the following one:

Now, you should have small thumbnails on desktop in any topic list (latest, a specific category, etc), and a “tiled” presentation on mobile with large thumbnails:

Canapin · May 10, 2022, 11:13pm

I have a little issue with my user profile, I guess this is a small bug:

Since it mentions the thumbnail, I suppose this is related to this theme component.

Still playing with this theme component by the way… And it’s awesome.

I have a question. Is it possible to prevent a topic from having a thumbnail, though the topic actually contains some?

Here’s my case:
I have a documentation category for which thumbnails are welcome.
But I also have a topic that just gives general advice when creating a new topic:

There is no meaningful image in it, but it adds a thumbnail automatically:

The only way I see to circumvent the issue is adding a random image at some point in the topic, and setting it as the thumbnail.

Example:

(but I admit it looks nice though…)

merefield · May 11, 2022, 6:24am

Oh yes that localisation is failing and needs to be fixed.

No, if there’s an image it will try to use one. Adding an additional nice one is the perfect workaround

This will look better for the consistency of your page layout too.

In any case I often use a featured topics Component (the built-in TLP one or another) to summarise those kind of posts at the top so having an image is quite nice.

raghukamath · May 14, 2022, 5:02am

I am trying to restrict the thumbnail and tiles to a specific tag in addition to certain categories but it is not working for tags. Here is my settings page i only want tiles and thumbnails for featured tag. Can you please tell me if I am doing something wrong.

Canapin · May 16, 2022, 12:00pm

There’s also a missing string in the plugin’s settings:

And another bug with the topic list excerpt removes links option.
So, as already reported, if I disable it there is no more link at all, even their text, as well as the “read more” button:

If I enable it, links in excerpts appear, as well as the “read more” link, but for some reason,

The read more link isn’t styled as a link (also already reported, but since it’s all related to the same option, I prefer to group all the issues at once)

image1160×1002 90.8 KB
Some excerpts are wrongly wrapped. Some examples:
Only the first part of the sentence from the excerpt is wrapped here:

image799×648 204 KB

image796×537 181 KB

An empty line is wrapped as an excerpt:

image795×252 109 KB

I’d be happy to help more but unfortunately, I don’t know anything about plugins and most of Discourse’s code…

merefield · May 16, 2022, 12:10pm

that looks like a bug, I’ll have a look soon.

merefield · May 16, 2022, 4:37pm

I’ve fixed this on the beta branch, could you please confirm things seem to be good now then I will merge.

For this to work you have to remove tag and tag-mobile and add the specific tag to the tag list setting.

(Install the beta version as another component and associate it with a test theme).

It turned out to be a breaking change in core.

Use the advanced button to reveal the branch and type in beta:

If I don’t hear from you within a while I’ll merge it anyway, but here is your chance to test and bring that forward.

merefield · May 16, 2022, 4:43pm

This is only gained by doing , a little bit at a time and picking up pieces of knowledge along the way

Canapin · May 21, 2022, 12:42pm

There’s also a JS error when we navigate on the website if the theme component is installed.

It’s also visible on your own website: https://starzen.space/
Click on any post and look at the JS console.

TypeError: Cannot read properties of null (reading 'querySelector')

The error is triggered on this Discourse file line:

github.com

discourse/discourse/blob/20d1f90edf490f17d02a1c6c00e3e5eb96366a26/app/assets/javascripts/discourse/app/components/topic-list-item.js#L338

      
        
                if (this.element && !this.isDestroying && !this.isDestroyed) {
                  this._mainLinkElement().classList.remove("focused");
                }
              },
            
            
  _shouldFocusLastVisited() {
                return !this.site.mobileView && this.focusLastVisitedTopic;
              },
            
            
  _mainLinkElement() {
                return this.element.querySelector(".main-link");
              },
            
            
  _titleElement() {
                return this.element.querySelector(".main-link .title");
              },
            });

merefield · May 21, 2022, 1:23pm

Looks like a breaking change in core: FIX: Don't listen for focus/blur events if the topic-list opts out of… · discourse/discourse@97e7bb1 · GitHub

Mainly fixed with: COMPATIBILITY: add css class to tiles to support focus · merefield/discourse-topic-list-previews-theme@4f0f0f0 · GitHub

Fix on beta branch as demo’d on my site.

Advantage of fixing this the way is we get the last visited indicator on the tile now

(Most of the errors disappears in mobile mobile too, but this indicator is not normally visible on mobile in any case, so I’m going to call this fixed!)

I will need to follow up on the less frequent error related to the title element.

merefield · May 21, 2022, 5:13pm

This is fixed in beta: FIX: missing localisation on user prefs and update locale paths

Canapin · May 21, 2022, 9:35pm

Thanks!

Regarding the plugin topic list excerpt remove links option, may I suggest changing the regex you currently use?

URI::regexp will remove any string that has a link pattern, which is not, in my opinion, always desirable.
Links have sometimes the same text content as their href value. That’s even a very common case in any forum, and removing the text content can lead to bizarre excerpts with sentences that make no sense.
This regex will also remove words that are inside or outside links sometimes. I give examples below.

Here’s a comparison with another regex: <a .+?\>(.+)?<\/a>
This is just an example regex; I understand that URI::regexp covers links in a more complex (and supposedly more reliable) way.

Here’s an example post:

And the resulting excerpts in the topic list:

URI::regexp
↓

(the facebook link text content is removed, and it also removed the word “Day” in “Astronomy Picture of the Day” for an unknown reason )

<a .+?\>(.+)?<\/a>
↓

Another example, just about a link text content removed that makes the excerpt weird to read:

URI::regexp
↓

(“the link was This may be my favorite halo”…?)

<a .+?\>(.+)?<\/a>
↓

(now that makes sense)

So, I’m not suggesting <a .+?\>(.+)?<\/a> is better, it was just a quick test, but I’m not sure using URI::regexp is the best choice regarding the results, both for the two reasons I mentioned: link text content vanishes, making excerpts weird sometimes, and also it removes words inside or outside links from time to time for an obscure reason. The latter issue seems frequent enough not to be negligible.

merefield · May 22, 2022, 8:03am

I appreciate the downsides of the current method (including the surprisingly weird over enthusiasm of the current algorithm where it deletes too much) but we arrived here through experience.

The main reason this option exists is actually to:

preserve formatting of preview when links are very long (ie delete all links so a long link will never leak into the excerpt to cause problems). Try the scenario where a link is very very long. The old support topic is littered with issues with Topic posts with long links.
preserve the excerpt as a predictable click surface where it will always navigate to the Topic. The text is too small for this to be discretionary.

Also should be:

simple to maintain and not cause support noise. Because the current method is a supported utitlity class I don’t have to worry about maintaining it.

I’m not yet convinced we need to change this for the general public?

I might be open to making this a selection of three options, OFF, no links (ie current method) and experimental? PR welcome. Let me move the current sidecar code to the master plugin branch first. I’ll try to do that this week.

Canapin · May 22, 2022, 11:46am

Thank you for your reply

I fixed the last sentence of my previous post, I forgot a word…
I wrote

and also it removes words inside or outside links from time to time for an obscure reason. The latter issue seems frequent enough to be negligible."

I forgot a “not”, so…

and also it removes words inside or outside links from time to time for an obscure reason. The latter issue seems frequent enough not to be negligible.

I might not understand much code stuff but I’ll try to understand and fix the issue number 2 I mentioned here: Topic List Previews Theme Component - #110 by Canapin today.

(the excerpts aren’t properly wrapped by .topic-excerpt: the wrapper seems to close just before the first link present in the excerpt instead of the end of the excerpt)

merefield · May 22, 2022, 12:07pm

Tbh the best approach might be we raise an issue with the maintainer of that utility and get them to fix it? It’s certainly not behaving as you’d expect?

Yeah haven’t looked at that yet. Feel free to PR a fix in meantime.

Canapin · May 22, 2022, 1:49pm

Thanks, I haven’t thought of that, I had no idea where this utility came from.

After searching for a bit, basically the regex recognizes pretty much any word or string as a part of a URI scheme as long as it’s immediately followed by a colon -ie: no space in between- which is understandable, but also a bit too much as in the English language (contrary to the French language, for example), we do not put space between a word and a colon. Hence “legit” words being eaten by the regex just because they’re unfortunately followed by a colon.

A fix could be having a field (not pre-filled to prevent any harm in existing installations) in the plugin settings to enter the scheme(s) we want to remove.

For example, the setting could be:

URI schemes to remove: http|https|ftp|mailto

That would result in:

#{URI::regexp(['http', 'https', 'ftp', 'mailto'])}
(but it’s, unfortunately, case sensitive, but that can surely be tweaked in some way)

If the setting is empty, then this is what would be used:

#{URI::regexp}

Being the current behavior.

Would a pull request with such a setting be welcomed?