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.

Sign up for GitHub

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

Jump to bottom

[MM-56695] Consolidate desktop and mobile Notifications to one and a new desktop notification sound settings #26198

Merged
merged 50 commits into from
Apr 8, 2024
Merged

[MM-56695] Consolidate desktop and mobile Notifications to one and a new desktop notification sound settings #26198

merged 50 commits into from
Apr 8, 2024

Conversation

M-ZubairAhmed
Copy link
Member

@M-ZubairAhmed M-ZubairAhmed commented Feb 12, 2024 •
edited

Summary

Changes

  • Combines Desktop and Mobile notifications settings into single section of the setting
  • Moves Desktop and Calls sounds settings into separate section

Bugs

  • Fixes If user selects 'Never' the 2 section divider stay on UI

Ticket Link

https://mattermost.atlassian.net/browse/MM-56695
https://mattermost.atlassian.net/browse/MM-56622
https://mattermost.atlassian.net/browse/MM-56696

Screenshots

Screenshot 2024-03-12 at 6 25 19 PM Screenshot 2024-03-12 at 6 26 43 PM

Release Note

Combine Desktop and Mobile notifications in user settings modal.

@mm-cloud-bot
Copy link

@M-ZubairAhmed: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

I understand the commands that are listed here

@M-ZubairAhmed M-ZubairAhmed changed the base branch from master to MM-56695 February 12, 2024 13:49
@M-ZubairAhmed M-ZubairAhmed changed the title Mm 56609 headers [MM-56695] Consolidate Desktop and Mobile Notifications to "Desktop and Mobile Notifications" Feb 12, 2024
@mm-cloud-bot mm-cloud-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed labels Feb 12, 2024
@M-ZubairAhmed
Copy link
Member Author

/update-branch

@M-ZubairAhmed M-ZubairAhmed marked this pull request as ready for review February 14, 2024 17:45
@abhijit-singh abhijit-singh added the Setup Cloud Test Server Setup an on-prem test server label Feb 14, 2024
@M-ZubairAhmed M-ZubairAhmed removed the Setup Cloud Test Server Setup an on-prem test server label Feb 15, 2024
Base automatically changed from MM-56695 to master February 15, 2024 09:56
@M-ZubairAhmed
Copy link
Member Author

/update-branch

@M-ZubairAhmed M-ZubairAhmed added Do Not Merge Should not be merged until this label is removed Setup Cloud Test Server Setup an on-prem test server labels Feb 15, 2024
marianunez
marianunez reviewed Feb 15, 2024
View reviewed changes
Copy link
Member

@marianunez marianunez left a comment

Choose a reason for hiding this comment

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

Thanks @M-ZubairAhmed! Looking good - some initial observations:

  • In the case they select Use different setting for mobile and in Desktop we have it set to Nothing the second dropdown for mobile asking if they want to be notified Away or Offline should still show correct? @abhijit-singh ?
Screenshot 2024-02-15 at 3 19 53 PM Screenshot 2024-02-15 at 10 18 34 AM

@mattermost-build
Copy link
Contributor

A pipeline with the same environment variables is already running.
Please cancel it first with /e2e-cancel, or specify different environment variables.

@unified-ci-app
Copy link
Contributor

unified-ci-app bot commented Apr 8, 2024

E2E test triggered successfully for PR #26198

@M-ZubairAhmed
Copy link
Member Author

/e2e-cancel

@mattermost-build
Copy link
Contributor

Looks like nothing had to be canceled.

@M-ZubairAhmed
Copy link
Member Author

/e2e-test

@mattermost-build
Copy link
Contributor

A pipeline with the same environment variables is already running.
Please cancel it first with /e2e-cancel, or specify different environment variables.

@unified-ci-app
Copy link
Contributor

unified-ci-app bot commented Apr 8, 2024

E2E test triggered successfully for PR #26198

@mm-cloud-bot
Copy link

Test server creation failed. Review the error details here.

@mattermost mattermost deleted a comment from mattermost-build Apr 8, 2024
@mattermost mattermost deleted a comment from unified-ci-app bot Apr 8, 2024
@mattermost mattermost deleted a comment from mattermost-build Apr 8, 2024
@mattermost mattermost deleted a comment from unified-ci-app bot Apr 8, 2024
@yasserfaraazkhan
Copy link
Contributor

/e2e-tests

@mattermost-build
Copy link
Contributor

Successfully triggered E2E testing!
GitLab pipeline | Test dashboard

@unified-ci-app
Copy link
Contributor

unified-ci-app bot commented Apr 8, 2024

E2E test triggered successfully for PR #26198

cwarnermm
cwarnermm reviewed Apr 8, 2024
View reviewed changes
Copy link
Member

@cwarnermm cwarnermm left a comment

Choose a reason for hiding this comment

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

I flagged some instances where a semi-colon was followed by a capitalized word (which is unconventional).

If you intended separate sentences with the capitalization, I recommend using periods over semi-colons.

...src/components/user_settings/notifications/desktop_and_mobile_notification_setting/index.tsx Outdated Show resolved Hide resolved
...src/components/user_settings/notifications/desktop_and_mobile_notification_setting/index.tsx Outdated Show resolved Hide resolved
webapp/channels/src/i18n/en.json Outdated Show resolved Hide resolved
webapp/channels/src/i18n/en.json Outdated Show resolved Hide resolved
webapp/channels/src/i18n/en.json Outdated Show resolved Hide resolved
webapp/channels/src/i18n/en.json Outdated Show resolved Hide resolved
webapp/channels/src/i18n/en.json Outdated Show resolved Hide resolved
@cwarnermm cwarnermm added 4: Reviews Complete All reviewers have approved the pull request and removed 2. Editor review Requires review by content editor labels Apr 8, 2024
@M-ZubairAhmed M-ZubairAhmed enabled auto-merge (squash) April 8, 2024 18:32
@M-ZubairAhmed M-ZubairAhmed merged commit 9698cfc into master Apr 8, 2024
42 of 44 checks passed
@M-ZubairAhmed M-ZubairAhmed deleted the MM-56609-headers branch April 8, 2024 18:59
@amyblais amyblais added Docs/Needed Requires documentation Changelog/Done Required changelog entry has been written labels Apr 8, 2024
@jwilander jwilander added the kind/refactor Categorizes issue or PR as related to refactor of production code. label May 7, 2024
@cwarnermm cwarnermm mentioned this pull request May 10, 2024
Merged
@amyblais amyblais added Docs/Done Required documentation has been written and removed Docs/Needed Requires documentation labels May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abhijit-singh abhijit-singh abhijit-singh approved these changes

@cwarnermm cwarnermm cwarnermm left review comments

@marianunez marianunez marianunez approved these changes

@yasserfaraazkhan yasserfaraazkhan yasserfaraazkhan approved these changes

@harshilsharma63 harshilsharma63 harshilsharma63 approved these changes

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Done Required changelog entry has been written Docs/Done Required documentation has been written kind/refactor Categorizes issue or PR as related to refactor of production code. release-note Denotes a PR that will be considered when it comes time to generate release notes.
Projects
None yet
Milestone
v9.8.0
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@M-ZubairAhmed @mm-cloud-bot @abhijit-singh @mattermost-build @yasserfaraazkhan @cwarnermm @marianunez @harshilsharma63 @jwilander @amyblais