-
-
Notifications
You must be signed in to change notification settings - Fork 595
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
Fix mark as unread button #3393
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.
I'm afraid I don't understand.
It's not helped by the fact that mkThread
is also undocumented (and uses a bunch of additional undocumented methods), but that's not necessarily an issue to solve right now.
spec/unit/room.spec.ts
Outdated
const { rootEvent, thread } = mkThread({ | ||
room, | ||
client: new TestClient().client, | ||
authorId: "@bob:example.org", | ||
participantUserIds: ["@bob: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.
It seems to me that adding this here means that we are no longer testing the behaviour when there are no thread events? Or is that tested somewhere else?
I'm unclear what exactly addRoomMainAndThreadMessages
is supposed to do, and what the semantics of tsMain
and tsThread
are supposed to do. Given the fact that we need to change this, I'm evidently not alone.
Please could you give the function a proper doc comment describing what it does, and what tsMain
and tsThread
do.
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.
@richvdh take a look at the last commit. I threw away the magic setup function. It should (hopefully) be clearer now.
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.
it's certainly much clearer now, thanks! Still a couple of questions, I'm afraid
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.
lgtm
it seems like there are outstanding questions about why exactly the Room is getting into these logically-inconsistent states, but that is probably a question for another day
* Fix mark as unread button * Revert to prefer the last event from the main timeline * Refactor room test * Fix type * Improve docs * Insert events to the end of the timeline * Improve test doc (cherry picked from commit 063d69e)
fixes element-hq/element-web#25411
PSF-2018
Checklist
This change is marked as an internal change (Task), so will not be included in the changelog.