Hi.
On short topics, the timeline is overlapping the “Suggested Topics” section:
I guess the timeline could be hidden on topics without any replies, or the posts container minimum height should be about the same as the timeline?
Hi.
On short topics, the timeline is overlapping the “Suggested Topics” section:
I guess the timeline could be hidden on topics without any replies, or the posts container minimum height should be about the same as the timeline?
As I recall, this has been around a while and is endemic to short screens and single-post one line topics, but perhaps we regressed?
Definitely seeing this.
@awesomerobot reminded me that we used to suppress the timeline for single post topics, I think? Perhaps that’s why we’re seeing it more, perhaps it is an actual regression? All the timeline elements are duplicated in the controls at the bottom of the first (and only) post in this case, anyway…
Yeah I think we’ve been hiding it since 2016: https://github.com/discourse/discourse/commit/b9f82641b04fd1b879075feaa80931f830b063c2
Until sometime recently, the timeline hid
@joffreyjaffeux could your recent change have caused this to regress?
https://github.com/discourse/discourse/commit/a947b7b839cfdd7436d533752d4df3eee1ce8bec
We should definitely hide the timeline when there is only ONE post. Three seems a bit much. But one…
The huge problem with 1 post is that when it is actually useful like this post, it does not work right. We don’t support navigation half way through a giant post so it gets very very very very confusing. Way worse than just the rendering issue.
Do it based on post length plus single post then… we don’t need a vertical timeline for a one sentence single post in a single topic, do we?
Absolutely not, but we need to fix the implementation, I guess that is what I am trying to get at.
If I had to choose between removing it unconditionally, or keeping it for outlier very long single posts I choose remove it unconditionally.
If we fix the underlying implementation though I would totally support keeping it around for @Johani’s amazing guide, it would be super helpful.
Logic should be
if single post in topic AND if single post is less than … 1000* chars … suppress timeline entirely
Even two one sentence posts is probably safe, maybe?
* but what if they are all single characters and CR/LFs etc I dunno. I think a rough check is fine, we don’t need to worry about insane edge cases here.
Sure we can do something here even just using CSS. But we got to fix scrollbar if we go for this approach.
Otherwise we might as well just change it to if 1 post then hide timeline
which is even simpler.
Except if that single post is a freakish 30,000 characters we don’t want it hidden, so… no.
We do want it hidden… sorry … that is my point. Unless we fix this issue:
I drag timeline down:
It is still at the top.
This scrollbar though works as expected if I use the native browser one:
If we leave it around for topics with 1 giant post it simply does not work right, which is very confusing. So we have to fix that.
I see, we need good informal test cases to make sure we’ve covered the ones we care about.
No this is not the commit.
the related commit is most probably:
https://github.com/discourse/discourse/commit/a3345057f2a14b25cf8f436e248d17e78502a8ce
So before this commit, the logic was kinda duplicated and different, and was also causing an empty (but visible) timeline-controls widget.
Will have a look today.
My attempt to fix the timeline overlapping is here:
https://github.com/discourse/discourse/pull/9581
It doesn’t fix what you explained, that clicking/dragging on timeline is not changing window scrollPosition. I quickly looked at doing this, and I think this should be a dedicated project, it has always been like this and changing this has MANY implications.
If we have only one post I will now check the height of the post, and hide scroller if it’s less than 1000px.
It represents a post of about this length:
Example with a short post:
This commit also fixes another bug, which was causing the topic admin button to show at an incorrect position on full page refresh:
Like this:
Instead of this position after fix:
I also checked that 2 posts with the minimum amount of content are not overlapping:
Nothing should have changed on mobile.
Excellent thank you Joffrey
This is merged, can be seen in action on this topic for example (at least as long as there’s only one post):
This topic was automatically closed after 5 days. New replies are no longer allowed.