Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Add options to pin unread/mentioned rooms to the top of the room list #1936

Merged
merged 4 commits into from
Nov 1, 2018

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented May 27, 2018

@turt2live turt2live force-pushed the travis/pinned-room-list branch from 6bd6add to 52cdf60 Compare May 27, 2018 02:30
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

@lukebarnard1 you might want to take a look at this is your room list store stuff. From my PoV

  • I dislike that this logic for room ordering is now split between here (the component) and RoomListStore.
  • Surely a sort function would make more sense here, rather than making separate lists of rooms with unread / notifications, de-duplicating them then concatenating them?

Edit: have also commented on element-hq/element-web#732

@turt2live
Copy link
Member Author

Originally there was a sort in there, however the sort gets complicated due to the tri-state nature: mentioned, notified (unread), and unread. Having the fused list makes it a little easier to understand in my opinion.

@lukebarnard1
Copy link
Contributor

I dislike that this logic for room ordering is now split between here (the component) and RoomListStore.

Two stores updating the same view isn't the worst thing in the world, I don't think. In this case the settings store isn't fluxy but the model still roughly holds.

Having said that, the RoomListStore already imports a lot of stuff and there's a _recentsComparator that this could be done in. And there's something to be said about keeping all the sorting logic in the same place.

@turt2live turt2live requested a review from a team October 12, 2018 20:36
@turt2live turt2live removed their assignment Oct 12, 2018
@bwindels
Copy link
Contributor

@turt2live UserSettings.js is conflicting with develop.

@turt2live
Copy link
Member Author

@bwindels thanks. Should be good now

@bwindels bwindels self-assigned this Oct 30, 2018
src/stores/RoomListStore.js Show resolved Hide resolved
src/stores/RoomListStore.js Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants