matrix-react-sdk
matrix-react-sdk copied to clipboard
matrix-org
Defer removal of the last ghost read marker
When navigating to a room with unread messages, a green line "unread marker" appears to indicate where they start, but it quickly animates away. It would be much more convenient for that line to remain, and this PR attempts to implement just that. It also includes an initial commit to synthesize a transitionend event in browsers that don't understand the specified transition style (fixing a bug that is currently preventing their ability to collect the read markers).
Notes: Keep unread markers visible until they need to move.
I did my best to follow https://github.com/matrix-org/matrix-js-sdk/blob/master/CONTRIBUTING.md , but I'm not sure if I succeeded. I also couldn't find a way to run tests or even successfully compile, so help with that would be appreciated.
Here's what your changelog entry will look like:
✨ Features
- Keep unread markers visible until they need to move. (#7630). Contributed by @gibson042.
Before we get to review this, is there an issue in the tracker that would explain the problem you're trying to fix? If not, would you be able to write a little bit about what you're trying to achieve, the approach you took and why you believe that's important
@gsouquet I've updated the PR description to clarify. However, I'm not sure what tracker you're referring to... should I open an issue at https://github.com/vector-im/element-web/issues per https://github.com/matrix-org/matrix-react-sdk#github-issues ?
However, I'm not sure what tracker you're referring to... should I open an issue at https://github.com/vector-im/element-web/issues per matrix-org/matrix-react-sdk#github-issues ?
Yes
Hi @gibson042 - thank you for the contribution! I would say the description you have added is a helpful start to having a conversation about your change - there's probably no need to create a separate issue. However, a change like this might need to go to the Design team before being approved, so be ready for some delays and discussion...
You should be able to run the tests by typing:
yarn test
inside the matrix-react-sdk directory.
Make sure you've follow these instructions first: Setting up a dev environment
If it doesn't work, please ask for help in #element-dev:matrix.org Matrix room.
Thanks @andybalaam. I was able to get successful tests in matrix-js-sdk, matrix-react-sdk in this branch, and element-web, so at least the changes are not breaking anything fundamental.
@gibson042 I've clicked the approval to build a test version of the code, so people can play with it. I've also added this as needing review from the Design team. To make it easier for them, I'd recommend adding a tiny video or screenshot of how it looks to the description. (Even better would be to do before and after videos.)
@gibson042 it is totally awesome that you are contributing, thank you. I suspect this one might take a while to progress. If you want faster gratification, pick up a small bug that doesn't make visual changes and it can be merged by a dev reasonably quickly if it's uncontroversial.
Thanks very much @andybalaam. I'll see if I can get a demonstration, although that might be tricky because it depends upon a stream of messages from other people. I understand that this might take a while, and there's no rush on my side because I'm already addressing it by user script.
Here's a demonstration of the user script approximation (which differs a bit from this PR, most notably in replacing the current "animate away" behavior with "shrink and turn red" so I know it's still working).
https://user-images.githubusercontent.com/558581/172684222-baa93951-06a3-4ade-8ee5-4fb8712556fd.mp4
ffmpeg -i "input.mp4" -pix_fmt yuv420p -c:v libx264 -crf 18 -preset slow -c:a copy "reencoded.mp4"
Original video (corrupted playback in Firefox)
https://user-images.githubusercontent.com/1199584/155849006-c97886ff-b07d-4f51-a5b8-05d07cbf6373.mp4
Here's a demonstration of the user script approximation (which differs a bit from this PR, most notably in replacing the current "animate away" behavior with "shrink and turn red" so I know it's still working). gh-7630.mp4
Hi there, thanks for contributing. Unfortunately the video is broken so I am unable to see any changes you are proposing. Could you upload the video again? Or perhaps share screenshots? :)
@amshakal sure.
before room navigation
before marker removal
after marker "removal"
Thanks for sharing the screenshots. We tend to use the colour red to communicate an error or an alert state. I would suggest having the green line as is but make it static till the user scrolls again. This way we'll be staying true to our design language and make sure users are aware where they should catch up from. Would that work for you?
I don't really care about the color or other styling concerns, just that the marker does not vanish.
@gibson042 in order for us to be able to merge this, it'll need to meet the design team's requirements, for which @amshakal is part of.
If this PR can be updated according to @amshakal's suggestions then we can take another look.
@turt2live The red is something I added for demonstration purposes and is not included in this PR, so I don't think any changes are needed to the current state are needed. https://github.com/matrix-org/matrix-react-sdk/blob/da6b3bd32b9ef2fba6f51125726114ac13d11f23/src/components/structures/MessagePanel.tsx#L589-L595
@amshakal I think it'll be best to test this from the deployment rather than via screenshots.
I have been using the deployed version all morning, the line seems disappears after a while, unlike what was mention above/what was in the video.
I am fine with the implementation if:
- the line is green (which has been already discussed above)
- if the line disappears when the user interacts in the timeline - this could include actions like writing a message, reacting or scrolling.
We think this has rotted enough that we should just close it. We're not convinced it improves things, and it has no tests and many conflicts.