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

[MM-40825] Account setting to always land users at the newest messages in a channel #10402

Merged
merged 34 commits into from
Jul 5, 2022
Merged

[MM-40825] Account setting to always land users at the newest messages in a channel #10402

merged 34 commits into from
Jul 5, 2022

Conversation

kyeongsoosoo
Copy link
Contributor

@kyeongsoosoo kyeongsoosoo commented May 27, 2022 •
edited by jgilliam17
Loading

Summary

  1. Added an advanced_setting option unreadScrollPosition.
  2. If an option is set to start_from_newest, recent posts are loaded together when unread posts are loaded.
  3. If an option is set to start_from_newest, scroll is set to newest message.
  4. If an option is set to start_from_newest and scroll is at bottom, unreadWithBottomStart toast is shown.
  5. unreadWithBottomStart toast action moves scroll to the oldest unread message.

Ticket Link

Fixes mattermost/mattermost#19278
Jira ticket: https://mattermost.atlassian.net/browse/MM-40825

Related Pull Requests

  • Has server changes (please link here)
  • Has mobile changes (please link here)

Screenshots

2022-05-27.2.25.14.mov

Release Note

Added an advanced_setting option `unreadScrollPosition`

@mattermod
Copy link
Contributor

Hello @kyeongsoosoo,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

@mattermod mattermod requested review from a team and M-ZubairAhmed and removed request for a team May 27, 2022 07:42
@mattermod mattermod added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester Contributor labels May 27, 2022
@Willyfrog Willyfrog requested review from hmhealey and koox00 May 31, 2022 15:18
@jasonblais
Copy link
Contributor

Thanks @kyeongsoosoo for your contribution! 🎉

@esethna
Copy link
Contributor

esethna commented Jun 1, 2022

Awesome, great work @kyeongsoosoo! Will defer to dev for code review

@hmhealey hmhealey removed the request for review from M-ZubairAhmed June 2, 2022 15:57
@hmhealey hmhealey added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Jun 8, 2022
hmhealey
hmhealey reviewed Jun 8, 2022
View reviewed changes
Copy link
Member

@hmhealey hmhealey left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in reviewing this. It's a big change to a complicated and sensitive part of the code base, so I wanted to make sure I had time to understand it.

I still have to give this a test, but this logic looks pretty sound to me. I have a few questions, but they're just to make sure I understand 100% of what's happening here

components/post_view/post_list/index.js Outdated Show resolved Hide resolved
components/post_view/post_list/index.js Outdated Show resolved Hide resolved
components/post_view/post_list_virtualized/post_list_virtualized.jsx Outdated Show resolved Hide resolved
components/post_view/post_view.jsx Outdated Show resolved Hide resolved
packages/mattermost-redux/src/actions/posts.ts Show resolved Hide resolved
components/toast_wrapper/toast_wrapper.jsx Show resolved Hide resolved
@mattermod
Copy link
Contributor

Successfully triggered e2e testing!
https://git.internal.mattermost.com/qa/cypress-ui-automation/-/pipelines/213917

jgilliam17
jgilliam17 suggested changes Jul 1, 2022
View reviewed changes
Copy link
Contributor

@jgilliam17 jgilliam17 left a comment

Choose a reason for hiding this comment

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

@kyeongsoosoo
Manually re-tested, looks good.
E2E report shows few toast related failures. Can you please take a look and fix? Thanks 🙂

@kyeongsoosoo
Copy link
Contributor Author

@jgilliam17
Fixed e2e failure cases!

@mm-cloud-bot
Copy link

Mattermost test server updated with git commit 58df02b41c06efa8225f910220516b0c643ad24c.

Access here: https://mattermost-webapp-pr-10402.test.mattermost.cloud

@mm-cloud-bot
Copy link

New commit detected. SpinWick will upgrade if the updated docker image is available.

@mm-cloud-bot
Copy link

Mattermost test server updated with git commit c7f34a6013c44830a92a211ab94010e1fe1f410e.

Access here: https://mattermost-webapp-pr-10402.test.mattermost.cloud

@jgilliam17
Copy link
Contributor

/e2e-test

@mattermod
Copy link
Contributor

Successfully triggered e2e testing!
https://git.internal.mattermost.com/qa/cypress-ui-automation/-/pipelines/214066

jgilliam17
jgilliam17 approved these changes Jul 1, 2022
View reviewed changes
Copy link
Contributor

@jgilliam17 jgilliam17 left a comment

Choose a reason for hiding this comment

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

Thanks @kyeongsoosoo
E2E report looks good, no PR related failures.
Screen Shot 2022-07-01 at 3 46 03 PM
Screen Shot 2022-07-01 at 3 45 50 PM

@jgilliam17 jgilliam17 added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester Setup Cloud Test Server Setup a test server using Mattermost Cloud labels Jul 1, 2022
@mm-cloud-bot
Copy link

Test server destroyed

@jgilliam17
Copy link
Contributor

@koox00 Can you please check and approve the workflow and merge? Thanks 🙂

@Willyfrog Willyfrog added the AutoMerge used by Mattermod to merge PR automatically label Jul 5, 2022
@mattermod
Copy link
Contributor

Will try to auto merge this PR once all tests and checks are passing. This might take up to an hour.

@mattermod
Copy link
Contributor

Trying to auto merge this PR.

@mattermod mattermod merged commit db9c10b into mattermost:master Jul 5, 2022
@mattermod
Copy link
Contributor

Pull Request successfully merged
SHA: db9c10b

@mattermod mattermod removed the AutoMerge used by Mattermod to merge PR automatically label Jul 5, 2022
@amyblais amyblais added Changelog/Done Required changelog entry has been written Docs/Needed Requires documentation labels Jul 11, 2022
@amyblais amyblais added this to the v7.2.0 milestone Jul 11, 2022
@cwarnermm cwarnermm mentioned this pull request Jul 18, 2022
Merged
@amyblais amyblais added Docs/Done Required documentation has been written and removed Docs/Needed Requires documentation labels Jul 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hmhealey hmhealey hmhealey approved these changes

@koox00 koox00 koox00 approved these changes

@esethna esethna esethna approved these changes

@jgilliam17 jgilliam17 jgilliam17 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 Contributor Docs/Done Required documentation has been written release-note
Projects
None yet
Milestone
v7.2.0
10 participants
@kyeongsoosoo @mattermod @jasonblais @esethna @hmhealey @jgilliam17 @mm-cloud-bot @koox00 @Willyfrog @amyblais