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

Room List sorting algorithms #4085

Merged
merged 18 commits into from
Feb 28, 2020
Merged

Room List sorting algorithms #4085

merged 18 commits into from
Feb 28, 2020

Conversation

t3chguy
Copy link
Member

@t3chguy t3chguy commented Feb 18, 2020 •
edited
Loading

Fixes element-hq/element-web#12242

image

image

when turning off the top two toggles I get:

image

@t3chguy
Remove unused props
8087f18 
Signed-off-by: Michael Telatynski <[email protected]>
@t3chguy
First attempt alpha room list sorting
932c9b3 
Signed-off-by: Michael Telatynski <[email protected]>
@t3chguy
Merge branches 'develop' and 't3chguy/alpha_room_list' of github.com:…
4278d44 
…matrix-org/matrix-react-sdk into t3chguy/alpha_room_list
@t3chguy
small tweaks, still not working reliably
d70a9bd 
Signed-off-by: Michael Telatynski <[email protected]>
@t3chguy
Iterate things
a6edc34 
Signed-off-by: Michael Telatynski <[email protected]>
@t3chguy
fix edge case
d1503d8 
Signed-off-by: Michael Telatynski <[email protected]>
@t3chguy
remove unused clause
5a31b30 
Signed-off-by: Michael Telatynski <[email protected]>
@t3chguy
tidy up
f074e85 
Signed-off-by: Michael Telatynski <[email protected]>
@t3chguy t3chguy marked this pull request as ready for review February 19, 2020 16:51
@t3chguy t3chguy requested a review from a team February 19, 2020 17:01
dbkr
dbkr approved these changes Feb 20, 2020
View reviewed changes
Copy link
Member

@dbkr dbkr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@turt2live might want to look too with his settings hat on

edit: actually, is augmentValue used anywhere?

@turt2live turt2live self-requested a review February 20, 2020 19:18
@turt2live
Copy link
Member

(will take a look, but I imagine it's fine - if you're confident then merge without my review)

@ara4n ara4n requested a review from a team February 20, 2020 20:33
@ara4n
Copy link
Member

ara4n commented Feb 20, 2020

this isn't behind a labsflag, but it's still a feature we want to get right for mozilla (and others). hence asking @nadonomy to review

@aaronraimist
Copy link
Contributor

To me it seems a bit confusing to have "Order rooms by message activity instead of by name" as a yes/no option.

Wouldn't it make more sense to say have a dropdown or picker?

Order rooms by:

  • Room name/alphabetical
  • Message activity

That would also allow for more sorting options to be added to the list in the future

@nadonomy
Copy link
Contributor

nadonomy commented Feb 21, 2020 •
edited
Loading

To me it seems a bit confusing to have "Order rooms by message activity instead of by name" as a yes/no option.

Wouldn't it make more sense to say have a dropdown or picker?

Order rooms by:

  • Room name/alphabetical
  • Message activity

That would also allow for more sorting options to be added to the list in the future

Yup, this is just for brevity, ease of implementation & consistency with existing interactions in Settings today.

We have a Settings redesign in the near future where we'll move this (and a bunch of other things) into more sensible interactions then, likely radio.

@t3chguy
Merge branches 'develop' and 't3chguy/alpha_room_list' of github.com:…
e6d8c4a 
…matrix-org/matrix-react-sdk into t3chguy/alpha_room_list
@nadonomy
Copy link
Contributor

Struggling to dogfood this on https://riots.im/adhoc/alpha1/, with both of the first 2 toggles disabled, I'm seeing:

Screenshot 2020-02-21 at 15 51 45

@t3chguy any ideas?

@ara4n
Copy link
Member

ara4n commented Feb 21, 2020

i am finding this very confusing to dogfood too. i wonder if we need some icon on the rooms to try to distinguish between rooms hoisted due to importance versus alphabet? or even a button on the section heading to toggle alpha sort on/off?

@jryans
Copy link
Collaborator

jryans commented Feb 24, 2020

Hmm, this seems to work well for me when disabling both of the top 2 options so that sorting is by name and unread notifs don't affect it:

2020-02-24 at 21 37

@nadonomy, were you testing with unread notif sort (the 2nd option) off or on?

@jryans jryans added the blocked label Feb 25, 2020
@jryans
Copy link
Collaborator

jryans commented Feb 25, 2020

This blocked for the moment: @nadonomy is planning to do some more testing and tweaking of the controls.

turt2live
turt2live suggested changes Feb 25, 2020
View reviewed changes
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

Requesting changes on the basis that we don't see to need anything fancy from the settings structure this time around.

src/settings/controllers/SettingController.js Outdated Show resolved Hide resolved
@turt2live
Copy link
Member

also sorry about the merge conflicts

@t3chguy
Copy link
Member Author

t3chguy commented Feb 26, 2020

image

@t3chguy
Merge branch 'develop' of github.com:matrix-org/matrix-react-sdk into…
a1908a2 
… t3chguy/alpha_room_list

� Conflicts:
�	src/components/views/rooms/RoomList.js
�	src/stores/RoomListStore.js
@t3chguy
Use RoomList.orderAlphabetically as the inverse of RoomList.orderByRe…
3ad8b6e 
…cents for migration

Signed-off-by: Michael Telatynski <[email protected]>
@t3chguy
Move Room List preferences section to be first
ed650d5 
Signed-off-by: Michael Telatynski <[email protected]>
@t3chguy
i18n
cada584 
Signed-off-by: Michael Telatynski <[email protected]>
@t3chguy
Copy link
Member Author

t3chguy commented Feb 26, 2020 •
edited
Loading

image

@t3chguy t3chguy requested a review from a team February 26, 2020 23:16
@t3chguy t3chguy removed the blocked label Feb 26, 2020
@t3chguy
remove the first preference section's margin
1875bf4 
Signed-off-by: Michael Telatynski <[email protected]>
@t3chguy
Fix copy
f036f0b 
Signed-off-by: Michael Telatynski <[email protected]>
@t3chguy
get rid of unused stuff
75c593b 
Signed-off-by: Michael Telatynski <[email protected]>
@t3chguy
fix default value for alphabetic sorting setting
e0fdfa1 
Signed-off-by: Michael Telatynski <[email protected]>
turt2live
turt2live approved these changes Feb 26, 2020
View reviewed changes
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

checkmark to signify the unused settings bits are removed - someone else will have to review the code itself.

@t3chguy
Copy link
Member Author

t3chguy commented Feb 27, 2020

/me requests another review as Travis cleared it from the queue

@t3chguy t3chguy requested a review from a team February 27, 2020 11:03
@jryans jryans removed the request for review from a team February 27, 2020 11:22
@jryans
Copy link
Collaborator

jryans commented Feb 27, 2020

(Cleared the product review as I assume the screenshot from @nadonomy above suffices as approval...?)

@t3chguy
Copy link
Member Author

t3chguy commented Feb 27, 2020

Yup

image

turt2live
turt2live approved these changes Feb 27, 2020
View reviewed changes
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

also seems fine code-wise

@t3chguy t3chguy merged commit 48dc671 into develop Feb 28, 2020
@t3chguy t3chguy deleted the t3chguy/alpha_room_list branch May 25, 2020 18:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@turt2live turt2live turt2live approved these changes

@dbkr dbkr dbkr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add option for room lists with a stable ordering (perhaps alphabetical)
7 participants
@t3chguy @turt2live @ara4n @aaronraimist @nadonomy @jryans @dbkr