FEATURE: Add last visit indication to topic view page. (PR #13471)

GitHub

This is to prevent the last visit line from shifting as new posts come in while the user is on the page.

Could we have a class for this please? I expect people might want to customise it.

I would like this change to run a bit deeper, I think part of it should be cleaning up topic_users, we no longer need both last_read_post_number and highest_seen_post_number we can drop the highest_seen_post_number column together with all the complexity that goes with it.

@SamSaffron I’ll prefer to do the server side clean up in another PR. Does that sound OK to you?

Sure go ahead and merge this (once the class is added) then work on the cleanup next

People can target it with .topic-post-visited-line span if they want to. Also, I’ll prefer if we add the class only when there is a need for it. We have .topic-list-item-separator with a td span element as well for the topic stream.

I’m sorry but no, if they want to style it, we need a class, directly styling elements cause lots of issues later in theming. This is an element which might get restyle and a such has “the need for it”.

I’ve added a class for the span.

1 Like

This pull request introduces 2 alerts when merging 5b9ad1ec6bb83a18709104107832e7104fc0c9e6 into 895df9c239db026c6a8b43db97092af58c1440f9 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

This doesn’t make sense to me in the current time and age of reading the code since the session prop highestSeenByTopic is set asynchronously while flushing our recently read data.

Don’t show last visit line on last post.

The thing am the most worried with this, is people overwriting templates. We should do a check on themes/plugins of big users to see if this will have any impact.

@jjaffeux Yea it looks like we have a bunch of internal themes that are overriding templates. I’ll submit PRs for them.

I am just wondering if it might be best to do this in a separate PR to make sure everything is all good? Otherwise we cannot get the data back if we need to for testing or whatever. Not a huge problem to leave it, just wondering.

Looks great and from what I can tell it is working fine locally. Just a couple of minor questions. It seems like all the SQL changes have been made accurately and tests are passing so good sign!

You might want to check on the tracking changes in https://github.com/discourse-org/discourse-teams-sidebar as well. From what I can see they should be fine, just another set of eyes would be good.

What has even changed here haha?

I have the same question/concern as Martin here, do we need to drop the column now? I see you’ve added the column to ignored_columns with a comment to drop the column in Jan next year, so I’m a little bit confused.

I grepped for the name of this component in all-the-themes/plugins and I see it and topic-post-badges.hbr are referenced in a few themes/plugins. This PR renames a couple of attributes that this component receives so we probably should go through official themes/plugins and update them.

Nah, we can easily get the data back if we need it, we can rebuild it from post_timings.