Skip to content
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

Removing notification settings v1 #4627

Merged
merged 3 commits into from
Dec 9, 2021

Conversation

ouchadam
Copy link
Contributor

@ouchadam ouchadam commented Dec 3, 2021

Removes the notification settings v1 code branches and flag

@ouchadam ouchadam requested a review from langleyd December 3, 2021 16:10
@ouchadam ouchadam mentioned this pull request Dec 3, 2021
Base automatically changed from feature/adm/feature-flags to develop December 3, 2021 16:38
@ouchadam ouchadam marked this pull request as ready for review December 3, 2021 16:41
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

You have done too much cleanup, see my other comment :)

lowPriorityClickListener { host.listener?.didSelectMenuAction(RoomListQuickActionsSharedAction.LowPriority(roomSummary.roomId)) }
}
// Preview, favorite, settings
bottomSheetRoomPreviewItem {
Copy link
Member

Choose a reason for hiding this comment

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

Test state.roomListActionsArgs.mode == RoomListActionsArgs.Mode.FULL should stay

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the previous condition was

// V2 always shows full details as we no longer display the sheet from RoomProfile > Notifications
val showFull = state.roomListActionsArgs.mode == RoomListActionsArgs.Mode.FULL || isV2

now that v2 is always true, do we need the condition (perhaps it was wrong previously 🤔 )

Copy link
Member

Choose a reason for hiding this comment

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

I do not think it was wrong before, it was OK in v1 mode, but with this PR, RoomListActionsArgs.Mode.NOTIFICATIONS is never used, so maybe just delete RoomListActionsArgs.Mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed c208c2d

if (showFull) {
RoomListQuickActionsSharedAction.Leave(roomSummary.roomId, showIcon = !isV2).toBottomSheetItem(5)
}
RoomListQuickActionsSharedAction.Leave(roomSummary.roomId, showIcon = !true).toBottomSheetItem(5)
Copy link
Member

Choose a reason for hiding this comment

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

So please also revert this change

Copy link
Member

Choose a reason for hiding this comment

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

(ignore)

Copy link
Member

Choose a reason for hiding this comment

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

I think more cleanup can be done in the fun RoomListQuickActionsSharedAction.toBottomSheetItem(...), there is some dead code now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch! removed ab04e40

@github-actions
Copy link

github-actions bot commented Dec 3, 2021

Unit Test Results

  66 files  ±0    66 suites  ±0   1m 3s ⏱️ +4s
135 tests ±0  135 ✔️ ±0  0 💤 ±0  0 ±0 
418 runs  ±0  418 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 96295f6. ± Comparison against base commit 147935d.

♻️ This comment has been updated with latest results.

@ouchadam ouchadam mentioned this pull request Dec 6, 2021
@ouchadam ouchadam force-pushed the feature/adm/removing-notification-settings-v1 branch from ab04e40 to 96295f6 Compare December 7, 2021 15:18
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Thanks for the update!

@bmarty bmarty merged commit 7c2bae3 into develop Dec 9, 2021
@bmarty bmarty deleted the feature/adm/removing-notification-settings-v1 branch December 9, 2021 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants