New sidebar feature fixed #602

csduarte · 2018-08-14T22:03:56Z

Summary

Bringing back the sidebar feature after a regression with the unread channels indicator on the web side.
Old PR: mattermost/mattermost-webapp#1374

Nothing has changed in the redux library.

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

Ran make check-style to check for style errors (required for all pull requests)
Ran make test to ensure unit and component tests passed
Added or updated unit tests (required for all new features)
Has redux changes (please link)
Has UI changes
Includes text changes and localization file (.../i18n/en.json) updates

hmhealey · 2018-08-31T19:49:16Z

src/selectors/entities/channels.js

 // If we receive an unread for a channel and then a mention the channel
 // won't be sorted correctly until we receive a message in another channel
 if (!currentUser) {
 return [];
 }

- const locale = currentUser.locale || General.DEFAULT_LOCALE;
- const allUnreadChannels = unreadIds.map((id) => {
+ const allUnreadChannels = unreadIds.filter((u) => u).map((id) => {


Why is this filter here? Is there a known case where we end up with a falsey id in unreadIds?

Last I check I was getting falsy ids. Some were null last I checked. I could check if that's still a problem

hmhealey · 2018-08-31T20:42:30Z

src/selectors/entities/channels.js

+ getCurrentUser,
+ getMyChannelMemberships,
+ getLastPostPerChannel,
+ (state, lastUnreadChannel, unreadsAtTop, favoritesAtTop, sorting = 'alpha') => sorting,


Can we get rid of the unused parameters for these? I see that lastUnreadPost and sorting are used, but getMapAndSortedUnreadChannelIds doesn't use unreadsAtTop and favoritesAtTop at all

hmhealey · 2018-08-31T20:46:31Z

src/selectors/entities/channels.js

+ getMapAndSortedUnreadChannelIds,
+ () => false,
+ () => false,
+ filterChannels,


Similarly, can we get rid of the extra argument functions for this selector? If you're concerned about passing the right number of arguments into filterChannels, you could do it like this to make it a bit clearer

export const getSortedUnreadChannelIds = createIdsSelector( getUnreadChannelIds, getFavoritesPreferences, getMapAndSortedUnreadChannelIds, (unreadChannelIds, favoriteChannelIds, mappedAndSortedUnreadChannelIds) => { return filterChannels(unreadChannelIds, favoriteChannelIds, mappedAndSortedUnreadChannelIds, false, false); } );

hmhealey · 2018-08-31T20:48:49Z

src/selectors/entities/channels.js

+ getCurrentUser,
+ getMyChannelMemberships,
+ getLastPostPerChannel,
+ (state, lastUnreadChannel, unreadsAtTop, favoritesAtTop, sorting = 'alpha') => sorting,


lastUnreadChannel, unreadsAtTop, and favoritesAtTop don't seem to be used by getFavoriteChannelIds

hmhealey · 2018-08-31T20:50:41Z

src/selectors/entities/channels.js

- getSortedFavoriteChannelWithUnreadsIds,
- filterUnreadChannels
+ getFavoritesPreferences,
+ getFavoriteChannelIds,


Instead of passing extra arguments to getFavoriteChannelIds that it doesn't use, you could remove the extra parameters from it and then change this to

(state, lastUnreadChannel, unreadsAtTop, favoritesAtTop, sorting) => getFavoriteChannelIds(state, sorting)

src/selectors/entities/channels.js

hmhealey · 2018-08-31T21:25:23Z

src/selectors/entities/sidebar.js

+ '',
+ null
+ );
+ return JSON.parse(sidebarPreference);


JSON.parse will return a new object each time, so it should be moved into the result function to prevent this from being memoized properly

hmhealey · 2018-08-31T21:26:08Z

src/selectors/entities/sidebar.js

+ sorting: 'alpha',
+};
+
+export const getSidebarPreferences = createSelector(


Could you move this into selectors/entities/preferences since sidebar isn't an entity?

miguelespinoza · 2018-09-05T15:53:13Z

Thanks for the feedback @hmhealey. I'm hoping to take a look at it today and tomorrow

miguelespinoza · 2018-09-12T18:31:27Z

Just a heads up, we had to put this on hold. Currently we have some top priority tasks for the upcoming weeks.
I'm hoping I can squeeze sometime next week to address this feedback but that's not certain

hmhealey · 2018-09-12T21:16:18Z

Thanks for the heads up. Definitely understand since we're doing the same for some other features.

sudheerDev

LGTM other than Harrison's comments

src/selectors/entities/channels.js

miguelespinoza · 2018-10-17T18:44:43Z

Hey @hmhealey, just a heads up. We plan on getting this in shape for merging. I'm addressing your feedback and would greatly appreciate any input for getting this in master soon. Thanks

hmhealey

I've got a couple other things that could be improved, but the rest of my concerns have been addressed

hmhealey · 2018-10-23T18:40:12Z

src/selectors/entities/channels.js

+ return getMapAndSortedUnreadChannelIds(state, lastUnreadChannel, sorting);
+ },
+ (unreadChannelIds, favoritePreferences, mappedAndSortedUnreadChannelIds) => {
+ return filterChannels(unreadChannelIds, favoritePreferences, mappedAndSortedUnreadChannelIds, false, false);


Since we're always passing false into these last two, can we skip that call entirely?

hmhealey · 2018-10-23T18:43:30Z

src/selectors/entities/channels.js

 getCurrentUser,
 getUsers,
 getAllChannels,
 getMyChannelMemberships,
 getUnreadChannelIds,
 getTeammateNameDisplaySetting,
- (state, lastUnreadChannel = null) => lastUnreadChannel,
- (currentUser, profiles, channels, myMembers, unreadIds, settings, lastUnreadChannel) => {
+ (currentUser, profiles, channels, myMembers, unreadIds, settings) => {


I don't think myMembers is needed here since the filtering has been removed

src/selectors/entities/channels.js

miguelespinoza · 2018-10-25T22:56:27Z

@hmhealey @sudheerDev thanks for the feedback. I've addressed this and will rebase the webapp PR tomorrow. Everything is good to go for the redux PR

hmhealey · 2018-10-30T15:42:12Z

Looks good. We're going to hold off on merging until the web app and mobile PRs are ready to avoid any compatibility issues

…lity

* New selectors for sidebar reorganization * Sort unread channels by mentions * Add test for new grouping and sorting algorithms * Add selector for sidebar preferences * Changes from feedback * Refactor sortByRecency function * Added channel selectors to include unreadIds, for backwards compatibility * Fix failing test for channel selector * Add global unread and favorite support * Unread section should respect sorting preference * Remove empty argument functions * Remove unused arguments * memoize getOrderedChannelIds * Move hasChannelsChanged outside of getOrderedChannelIds * Remove getOrderedChannelIdsDeprecated * Move getSidebarPreferences to selectors/entities/preferences * Move JSON to result function * Fix failing test * Remove string comparison for orderedChannels * Filter channels that have been deleted in getUnreadChannels * Remove unnecessary filterChannel call * Fix test for more_direct_channels selector * Fix issue with filter in getUnreadChannels * remove myMembers from getUnreadChannels * Sidebar preference overrides previous unread sorting

miguelespinoza force-pushed the sidebar-fixed branch 4 times, most recently from 7ad0e4c to 1d9747a Compare August 23, 2018 19:59

miguelespinoza mentioned this pull request Aug 28, 2018

New sidebar feature fixed mattermost/mattermost-webapp#1568

Merged

6 tasks

miguelespinoza force-pushed the sidebar-fixed branch from e8ed44c to dcdfde2 Compare August 28, 2018 22:02

esethna requested review from hmhealey and sudheerDev August 29, 2018 15:13

hmhealey suggested changes Aug 31, 2018

View reviewed changes

hmhealey added the 2: Dev Review Requires review by a core commiter label Aug 31, 2018

hmhealey added this to the v5.4.0 milestone Aug 31, 2018

sudheerDev approved these changes Sep 17, 2018

View reviewed changes

src/selectors/entities/channels.js Show resolved Hide resolved

enahum modified the milestones: v5.4.0, v5.6.0 Oct 3, 2018

miguelespinoza force-pushed the sidebar-fixed branch from dcdfde2 to 3ba020c Compare October 17, 2018 15:03

esethna assigned hmhealey and sudheerDev Oct 23, 2018

hmhealey suggested changes Oct 23, 2018

View reviewed changes

sudheerDev reviewed Oct 23, 2018

View reviewed changes

src/selectors/entities/channels.js Show resolved Hide resolved

miguelespinoza force-pushed the sidebar-fixed branch 2 times, most recently from bd3cb58 to 3a9437c Compare October 25, 2018 21:40

hmhealey added Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) and removed 2: Dev Review Requires review by a core commiter labels Oct 30, 2018

hmhealey approved these changes Oct 30, 2018

View reviewed changes

miguelespinoza added 22 commits November 8, 2018 12:22

Add test for new grouping and sorting algorithms

5374a72

Add selector for sidebar preferences

a683bb4

Changes from feedback

c64a017

Refactor sortByRecency function

e97bb52

Added channel selectors to include unreadIds, for backwards compatibi…

65586cf

…lity

Fix failing test for channel selector

a94fcff

Add global unread and favorite support

0cefa01

Unread section should respect sorting preference

3836c12

Remove empty argument functions

1a018a7

Remove unused arguments

35d9f7e

memoize getOrderedChannelIds

e615ce3

Move hasChannelsChanged outside of getOrderedChannelIds

65cdd24

Remove getOrderedChannelIdsDeprecated

7a9e0fb

Move getSidebarPreferences to selectors/entities/preferences

09239e0

Move JSON to result function

709174d

Fix failing test

60d94b1

Remove string comparison for orderedChannels

4211987

Filter channels that have been deleted in getUnreadChannels

d37ee80

Remove unnecessary filterChannel call

14b77fd

Fix test for more_direct_channels selector

6492aff

Fix issue with filter in getUnreadChannels

f779fa8

remove myMembers from getUnreadChannels

b0a415b

miguelespinoza force-pushed the sidebar-fixed branch from a201ddd to b0a415b Compare November 8, 2018 17:27

Sidebar preference overrides previous unread sorting

d5bb11b

enahum approved these changes Nov 9, 2018

View reviewed changes

hmhealey removed the Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) label Nov 9, 2018

hmhealey merged commit e86e83d into mattermost:master Nov 9, 2018

enahum added the 4: Reviews Complete All reviewers have approved the pull request label Nov 9, 2018

chetanyakan pushed a commit to brightscout-alpha/mattermost-redux that referenced this pull request Feb 9, 2021

MM-31713 added getUserThread method (mattermost#602)

cb5a60f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New sidebar feature fixed #602

New sidebar feature fixed #602

csduarte commented Aug 14, 2018

hmhealey Aug 31, 2018

miguelespinoza Oct 17, 2018 •

edited

Loading

hmhealey Aug 31, 2018

hmhealey Aug 31, 2018

hmhealey Aug 31, 2018

hmhealey Aug 31, 2018

hmhealey Aug 31, 2018

hmhealey Aug 31, 2018

miguelespinoza commented Sep 5, 2018

miguelespinoza commented Sep 12, 2018

hmhealey commented Sep 12, 2018

sudheerDev left a comment

miguelespinoza commented Oct 17, 2018

hmhealey left a comment

hmhealey Oct 23, 2018

hmhealey Oct 23, 2018

miguelespinoza commented Oct 25, 2018

hmhealey commented Oct 30, 2018

New sidebar feature fixed #602

New sidebar feature fixed #602

Conversation

csduarte commented Aug 14, 2018

Summary

Checklist

Choose a reason for hiding this comment

miguelespinoza Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguelespinoza commented Sep 5, 2018

miguelespinoza commented Sep 12, 2018

hmhealey commented Sep 12, 2018

sudheerDev left a comment

Choose a reason for hiding this comment

miguelespinoza commented Oct 17, 2018

hmhealey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguelespinoza commented Oct 25, 2018

hmhealey commented Oct 30, 2018

miguelespinoza Oct 17, 2018 •

edited

Loading