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

Comment depth #1072

Merged
merged 8 commits into from
Jun 15, 2023
Merged

Comment depth #1072

merged 8 commits into from
Jun 15, 2023

Conversation

SleeplessOne1917
Copy link
Member

Closes #1069. In addition to making comment threads lists to make them easier to use with screen readers, I also made it easier to track of comment depth visually per a user suggestion. This is what a thread looks like now:
image

@AgentElement
Copy link

User that suggested the change here -- thanks for the (very) quick PR! Hoping this gets merged soon.

: colorList[0];

return this.props.nodes.length > 0 ? (
<ul
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be an ordered list, since it's sorted by some criterion?

Copy link
Member Author

Choose a reason for hiding this comment

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

I could make it ordered. Whichever you think would be better for accessibility. However, I don't think children of top level comments get sorted, so that's worth keeping in mind.

Copy link

Choose a reason for hiding this comment

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

As @fastfinge said, ul makes more sense from a screen reader perspective. With an ol, you'd (depending on settings) get the item number and the number of items on the list: the comment number and the total number of comments at that depth, in that thread. That seems like a lot of extra info to process.

@fastfinge
Copy link

The issue with an ordered list is that it would cause screen readers to read out a number before each comment, and that doesn't seem to make much sense. It matters that the comment is at depth 2, not that it's the fifth comment in this thread at depth 2. Perhaps someone else disagrees, but I'm struggling to think of a reason I'd ever want to innumerate the order of comments. It's the depth that matters here.

@dessalines
Copy link
Member

dessalines commented Jun 5, 2023

The colored lines seem overkill now. Would you just make them gray / muted? Example:

Screenshot_2023-06-05-11-56-19-832_com rubenmayayo reddit

@Nutomic
Copy link
Member

Nutomic commented Jun 5, 2023

Colors seem fine but I would make them lighter or translucent.

@Kommynct
Copy link

Kommynct commented Jun 6, 2023

An option to click on them to collapse threads would also be good, but then you'd probably want to make them wider.

@dessalines
Copy link
Member

@Kommynct That's a separate issue. Please keep things on topic, we're juggling many things right now.

@nebeker
Copy link

nebeker commented Jun 6, 2023

While we're on an accessibility vibe, passing level AA here would be excellent for readability:
https://webaim.org/resources/contrastchecker/

@AgentElement
Copy link

We could have colored vs muted nesting indicators as an option in the settings, leaving them on (colored) by default. I think it's a matter of user preference.

@Nutomic
Copy link
Member

Nutomic commented Jun 7, 2023

Adding new options is pretty complicated, not a great idea for something so minor.

@Kommynct
Copy link

Kommynct commented Jun 7, 2023

I think colored is better for visibility, and if you like uncolored lines, I can easily whip up a stylus theme that colors them however you like. If they are uncolored I could probably not make them colored with the selectors.

I think we should prioritize accessibility over aesthetics.

@Junebugging
Copy link

Either way, colored or uncolored is a small difference, and can be addressed later on
For now I think the priority should be getting any variation of this PR merged, as there is a big influx of users, many of them laypeople, and whether they stay or leave depends partially on how good the UI & UX is, and this is a small but significant improvement to it.

@SleeplessOne1917
Copy link
Member Author

@Junebugging Now that the big HTTP changes are finally merged into main, I can finish this up.

@SleeplessOne1917
Copy link
Member Author

The comments look like this now.

image

@SleeplessOne1917
Copy link
Member Author

@dessalines @jsit @nebeker If you don't have any more requested changes, please approve.

@SleeplessOne1917 SleeplessOne1917 merged commit de4b1a0 into main Jun 15, 2023
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.

depth of threaded comments is not communicated to a screen reader
9 participants