Better topic map top link title for links to categories

pr-welcome

(Robert) #1

Hello,

let’s say I link to a category: #feature

The topic map top link will read /c/feature or something like that. It would be nicer to have a title like Feature Category.

PS: I noticed this in a private message.


(Erlend Sogge Heggen) #2

I don’t understand what the “The topic map top link” is


(Robert) #3

I have found that name somewhere in the forum. Here, it is under the caption “Popular Links”:


(Erlend Sogge Heggen) #4

Aaah okay. Yeh changing that title might make make sense.


(Katarzyna Bułat) #5

Hi! I’m new to Discourse and it is the first open source project I’m working on. Can I take this issue as my starter-task? I think it will help me to get to know the project.


(Régis Hanol) #6

Sure, go for it :+1: Do not hesitate if you need help.


(Katarzyna Bułat) #7

Thank you! I found the place, which needed a change, edited the code and created a pull request: UX: better topic map top link title for links to categories by kasiabulat · Pull Request #5289 · discourse/discourse · GitHub.


(Sam Saffron) #9

Hi @kasiabulat I was just reviewing this and sadly this is bit complicated, especially when localization is considered.

The bug here is the the title field in TopicLink is not being populated for these internal category links, the fix is fully server side not client side.

I did remove #starter-task from here cause it is somewhat complicated, but if you feel like giving it a shot again on the server side let me know … so sorry I need to close the open PR I feel extra bad about it. We should have guided you properly.


(Jeff Atwood) #10

I would just suppress category links from the topic map. I don’t think they add a lot. That would also be an easier PR I think @sam?


(Sam Saffron) #11

Marginally, cause only easy fix is to suppress all internal links, you don’t really want to write yet another route matcher client side.

We could suppress all internal links that don’t have a title, that would be somewhat easier, but then what if you link to TOS or something else that is important.

Lets test this out: https://meta.discourse.org/tos, Privacy Policy - Discourse Meta, /faq

Hmmm: yeah

<a class="hashtag" href="/c/2-feature">#<span>feature</span></a> yeah this thing needs a title which our CommonMark engine should provide and then a proper fix would flow.

Wow this is pretty messy for internal links:

image

looks like mini onebox is not setting title…


(Jeff Atwood) #12

Yeah simply adding title for internal links would be best there. I think you meant to say ‘topic map’ in your last sentence as well?


(Katarzyna Bułat) #13

OK, so I think I would like to coutinue working on it. Can you give me some tips on where can I begin and somehow summarize what should be done?


(Jeff Atwood) #14

@eviltrout may be able to advise he worked on it last.


(Sam Saffron) #15

Simplest change is to amend discourse/category-hashtag.js.es6 at master · discourse/discourse · GitHub it is already covered in tests, so they will need changing


(Katarzyna Bułat) #16

Thanks for the tip. Well, I could change the line 11: token.attrs = [['class', 'hashtag'], ['href', result[0]]] (in discourse/category-hashtag.js.es6 at master · discourse/discourse · GitHub), so that there is also title added as an argument (I guess that there is a funtion that changes slug to the category name, but I haven’t found it yet). If I do it that way, hashtags to new created categories (not all of them, so I think it doesn’t satisfy us) will look something like: <a class="hashtag" href="/c/1-category-name" title="Category Name">#<span>category-name</span></a> in post replies. And then what will be needed is getting this title out of the attributes and showing it the post summary (topic map). But the title from attributes is null for these hashtags in the place I tried to change in my previous pull request (discourse/topic-map.js.es6 at 3b7128102c12d72d0303c83e4f113aa06c23655c · discourse/discourse · GitHub, call of the function createWidget creating topic-map-link widget, line 127), so this title is probably changed by some other functions earlier and for now I don’t know how exactly it flows. Should I dig into it and analyze how it all works? And please correct me if I don’t undestand something in what I described.