-
-
Notifications
You must be signed in to change notification settings - Fork 829
Include a mark as read X under the scroll to unread button #4159
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code-wise seems fine, just needs design review and a sign-off.
Easiest way to do sign off would be to leave a comment here with the following template:
Signed-off-by: Your Name <email@example.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @mstriemer thanks for this! I think being able to dismiss the read marker is super useful.
Relatedly, the logic and interactions behind read markers in general could do with re-visiting, which we do plan to do, so these specific changes may end up being short lived if we arrive at a different more holistic solution. But, I don't see that as a reason to block this PR.
I can see some opportunities in sophisticating the mechanic to reveal itself when hovering over the up arrow, but we can dogfood this first and go from there.
LGTM!
Awesome, thanks Nad! @mstriemer please sign off on the changes and we can merge it :) |
@turt2live I don't see anything in the readmes about the sign-off. What's the purpose of it? Just want to know what I'm signing off on :) |
Through a series of clicking links, you'd eventually land on this: https://github.com/matrix-org/matrix-js-sdk/blob/master/CONTRIBUTING.rst#sign-off It's essentially to say you're okay with us merging the code, and that you have the right to submit it in the first place (ie: not stealing code from some other project and PRing it under your own name). |
Cool, thanks! I checked the readme for this repo which has a link to a contributing guide that 404s, I'm guessing it should link to that page instead?
|
ah, yes, that should be a useful link and not a useless one. Will get that fixed. Thanks! |
This adds a simple X button under the scroll to top button that will mark all as read.
Probably needs some tests or something, but I didn't want to invest too much into this if it wasn't the desired solution.
Looks like the functionality was nearly there but I might be missing something. Open to suggestions but mostly wanted to get something started here.
Fixes element-hq/element-web#8666.