Skip to content
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

show threads #350

Merged
merged 1 commit into from
Jun 30, 2019
Merged

show threads #350

merged 1 commit into from
Jun 30, 2019

Conversation

webkonstantin
Copy link
Contributor

@webkonstantin webkonstantin commented Jun 17, 2019

@Reeywhaar
Copy link
Collaborator

Wow, thank you so much!

At first glimpse I see problem: Comments on level 5 and below should not be indented (think of mobile), yet somehow show that they are replies 🤔

@umputun
Copy link
Owner

umputun commented Jun 17, 2019

I think the line should be aligned to the left side of the first symbol. I know Reddit does it in the middle of the first one, but they have arrows here.

Comments on level 5 and below should not be indented (think of mobile), yet somehow show that they are replies 🤔

I think the current indication on a deep level is fine but will be nice to fix it to actually jump to the parent comment

wbfcn-201906-17111237-qw6g2

@umputun
Copy link
Owner

umputun commented Jun 29, 2019

this is an example as NYT shows threads
7of6y-201906-29115551-hid8w

@webkonstantin
Copy link
Contributor Author

fixed alignment

@umputun
Copy link
Owner

umputun commented Jun 29, 2019

@sw-double & @Reeywhaar - what is the status of PR? is it still WIP or good to be merged?

@webkonstantin webkonstantin changed the title wip show threads show threads Jun 29, 2019
@webkonstantin
Copy link
Contributor Author

not sure that it handles dark theme in an idiomatic way

otherwise LGTM on my end

@umputun
Copy link
Owner

umputun commented Jun 29, 2019

@sw-double cool, thx!

@Reeywhaar - need your blessing

@Reeywhaar
Copy link
Collaborator

I'm working on ui and stream api, plan to end in next 3 hours, then I'll watch this PR.
@umputun hope you don't rush to make it before the show :-)
@sw-double, thank you so much 👍

@umputun
Copy link
Owner

umputun commented Jun 29, 2019

sure, take your time. thx

@Reeywhaar Reeywhaar mentioned this pull request Jun 29, 2019
}
.thread > .thread {
padding-left: 17px;
border-left: 1px solid rgba(0, 0, 0, 0.15);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it me, or lines color is too active in light theme?

Screenshot 2019-06-30 at 02 14 01

I'd rather mute it to rgba(0,0,0,.07)
Screenshot 2019-06-30 at 02 17 33

... or even to rgba(0,0,0,.05)
Screenshot 2019-06-30 at 02 14 15

... or another variant to make border dotted:
Screenshot 2019-06-30 at 02 13 29

.thread_level_4 .thread > .thread {
padding-left: 5px;
}
.comment_theme_dark ~ .thread {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather move this selector to comment_theme_dark.scss and make it like

.comment_theme_dark {
  ...
  & ~ .thread {...}
}

@Reeywhaar
Copy link
Collaborator

In general I see no crimes here, concise work, @sw-double, thank you, once again! So, @umputun, if you like it then great, I can fix some minor issues myself.

@Reeywhaar
Copy link
Collaborator

I think we should make something with mobile layout
Screenshot 2019-06-30 at 02 36 32

@umputun umputun merged commit e2237e7 into umputun:master Jun 30, 2019
@umputun
Copy link
Owner

umputun commented Jun 30, 2019

Generally, I like it, thx @sw-double . Two issues to address:

  1. All internals "levels" don't need any visible thread lines. On such a deep level it is already impossible to understand the tree and adding zebra doesn't make it any better
  2. I agree with @Reeywhaar - those lines are too visible. Your dotted variant looks better to me

4wxbw_20190629_191748 a4ra6

@Reeywhaar
Copy link
Collaborator

I'm afraid without the lines it becomes even more unclear
Screenshot 2019-06-30 at 03 50 06

... I can make level5+ lines even more muted
Screenshot 2019-06-30 at 03 52 04

@umputun
Copy link
Owner

umputun commented Jun 30, 2019

Is there a reason why 5+ level indentations are any different? Or why we even have any for such deep levels?

@Reeywhaar
Copy link
Collaborator

Not sure I follow. 5+ are indented less mainly for mobile and for big discussions. In old version, as you may remember, levels 5+ were unindented at all. Dunno what helps there actually, I think comments this deep are mess no matter what. What do you suggest?

@Reeywhaar
Copy link
Collaborator

One another solution is to set min-width on thread, and if some discussiton breaks this limit then we get horizontal scroll
ezgif-2-1289a7ea05e1

@umputun
Copy link
Owner

umputun commented Jun 30, 2019

My suggestion is to revert 5+ to unindented

@Reeywhaar Reeywhaar mentioned this pull request Jun 30, 2019
@Reeywhaar
Copy link
Collaborator

ok, agree

@webkonstantin webkonstantin deleted the threads branch July 1, 2019 12:57
@umputun umputun mentioned this pull request Jul 1, 2019
@umputun umputun added this to the v1.4 milestone Jul 27, 2019
@umputun umputun mentioned this pull request Jul 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants