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

[MM-24507] Affix comment input to bottom in right-hand side (RHS) #5470

Merged
merged 27 commits into from
May 20, 2020
Merged

[MM-24507] Affix comment input to bottom in right-hand side (RHS) #5470

merged 27 commits into from
May 20, 2020

Conversation

bradjcoughlin
Copy link
Contributor

Summary

Currently, the comment input box will scroll out of view on longer threads. This change moves the input out of the scrollbars so it stays visible at all times.

Ticket Link

https://mattermost.atlassian.net/browse/MM-24507

@bradjcoughlin bradjcoughlin added 1: PM Review Requires review by a product manager Setup Cloud Test Server Setup a test server using Mattermost Cloud labels May 11, 2020
@bradjcoughlin bradjcoughlin added the 3: QA Review Requires review by a QA tester label May 11, 2020
esethna
esethna suggested changes May 12, 2020
View reviewed changes
Copy link
Contributor

@esethna esethna left a comment

Choose a reason for hiding this comment

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

@bradjcoughlin this should only pin the box when there are enough posts in the RHS to fill it:
image

See ticket:
image

@esethna esethna removed the 3: QA Review Requires review by a QA tester label May 12, 2020
@bradjcoughlin
Copy link
Contributor Author

@esethna Yep, that's right. Back to the drawing board.

@bradjcoughlin bradjcoughlin removed the Setup Cloud Test Server Setup a test server using Mattermost Cloud label May 13, 2020
@bradjcoughlin
Copy link
Contributor Author

@esethna this is ready to test but the builds are failing. I'll check on it again in awhile.

@bradjcoughlin
Copy link
Contributor Author

/update-branch

@bradjcoughlin bradjcoughlin added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label May 13, 2020
@mm-cloud-bot mm-cloud-bot removed the Setup Cloud Test Server Setup a test server using Mattermost Cloud label May 13, 2020
@esethna esethna added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label May 14, 2020
@mm-cloud-bot mm-cloud-bot removed the Setup Cloud Test Server Setup a test server using Mattermost Cloud label May 14, 2020
@bradjcoughlin
Copy link
Contributor Author

/update-branch

@bradjcoughlin bradjcoughlin added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label May 14, 2020
esethna
esethna approved these changes May 14, 2020
View reviewed changes
Copy link
Contributor

@esethna esethna left a comment

Choose a reason for hiding this comment

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

Looks good other than this slight UI issue. See expected in center pane:

image

@esethna esethna removed the 1: PM Review Requires review by a product manager label May 14, 2020
@esethna esethna added this to the v5.24.0 milestone May 14, 2020
@esethna
Copy link
Contributor

esethna commented May 14, 2020

@bradjcoughlin ^

@bradjcoughlin
Copy link
Contributor Author

@esethna can you explain what's going on there? Is it getting cut off even when scrolled to the bottom?

@esethna
Copy link
Contributor

esethna commented May 14, 2020 •
edited
Loading

@bradjcoughlin there is a padding around the RHS input box that shouldn't be there (see the center pane in comparison). Only occurs when scrolled up in a thread

@bradjcoughlin
Copy link
Contributor Author

/update-branch

@bradjcoughlin
Copy link
Contributor Author

bradjcoughlin commented May 19, 2020 •
edited
Loading

@jgilliam17 I opened the test server in Safari and discovered that scrolling in the RHS isn't working at all. Investigating now.

@jgilliam17
Copy link
Contributor

@jgilliam17 I opened the test server in Safari and discovered that scrolling in the RHS isn't working at all. Investigating now.

Great catch, I missed that.
I checked Edge and desktop app on Win10 - no issue
Also, Chrome, Firefox and desktop app on macOS Catalina - no issue there either.

jgilliam17
jgilliam17 approved these changes May 20, 2020
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.

Thank you @bradjcoughlin
Tested, looks good to merge.

  • Verified comment input box is fixed to bottom in RHS, stays in view on extended threads.
    (Checked on Safari, Chrome, Edge & Firefox + Desktop)

@jgilliam17 jgilliam17 added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter Setup Cloud Test Server Setup a test server using Mattermost Cloud labels May 20, 2020
@mm-cloud-bot
Copy link

Test server destroyed

@bradjcoughlin
Copy link
Contributor Author

/update-branch

@bradjcoughlin bradjcoughlin merged commit 957c9a9 into mattermost:master May 20, 2020
@amyblais amyblais added the CherryPick/Approved Meant for the quality or patch release tracked in the milestone label May 20, 2020
@amyblais
Copy link
Member

@bradjcoughlin Looks like this wasn't cherry-picked yet.

@bradjcoughlin
Copy link
Contributor Author

/cherry-pick release-5.24

@mattermod
Copy link
Contributor

Error trying doing the automated Cherry picking. Please do this manually

+++ Updating remotes...
Fetching upstream
Fetching origin
+++ Updating remotes done...
+++ Creating local branch automated-cherry-pick-of-#5470-upstream-release-5.24-1590007876
Branch 'automated-cherry-pick-of-#5470-upstream-release-5.24-1590007876' set up to track remote branch 'release-5.24' from 'upstream'.
+++ Downloading patch to /tmp/5470.patch (in case you need to do this again)

+++ About to attempt cherry pick of PR. To reattempt:
  $ git am -3 /tmp/5470.patch

Applying: move comment box outside of scrollbars so it “sticks”
Using index info to reconstruct a base tree...
A	components/rhs_thread/rhs_thread.jsx
Falling back to patching base and 3-way merge...
Auto-merging components/rhs_thread/rhs_thread.tsx
Applying: update snapshot
Using index info to reconstruct a base tree...
A	components/rhs_thread/__snapshots__/rhs_thread.test.jsx.snap
Falling back to patching base and 3-way merge...
Auto-merging components/rhs_thread/__snapshots__/rhs_thread.test.tsx.snap
Applying: scrollbars autoheight, let scrollbars manage scrollToBottom
Using index info to reconstruct a base tree...
A	components/rhs_thread/rhs_thread.jsx
Falling back to patching base and 3-way merge...
Auto-merging components/rhs_thread/rhs_thread.tsx
CONFLICT (content): Merge conflict in components/rhs_thread/rhs_thread.tsx
Patch failed at 0003 scrollbars autoheight, let scrollbars manage scrollToBottom
Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

+++ Conflicts detected:

UU components/rhs_thread/rhs_thread.tsx

+++ Aborting in-progress git am.

+++ Returning you to the master branch and cleaning up.

bradjcoughlin added a commit that referenced this pull request May 20, 2020
@bradjcoughlin
[MM-24507] Affix comment input to bottom in right-hand side (RHS) (#5470
d38e54e 
)

* move comment box outside of scrollbars so it “sticks”
* update snapshot
* scrollbars autoheight, let scrollbars manage scrollToBottom
* update unit tests
* lint rolling : )
* remove padding above comment input
* scroll to bottom is RHS thread is refocused
* fix test for typescript
* type check errors
* add padding to rhs thread, typescript fixes
* use native scrollbars
@bradjcoughlin bradjcoughlin added CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone and removed CherryPick/Approved Meant for the quality or patch release tracked in the milestone labels May 20, 2020
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels May 20, 2020
@bradjcoughlin bradjcoughlin mentioned this pull request May 22, 2020
Closed
bradjcoughlin added a commit to bradjcoughlin/mattermost-webapp that referenced this pull request May 26, 2020
@bradjcoughlin
Revert "[MM-24507] Affix comment input to bottom in right-hand side (…
04c8873 
…RHS) (mattermost#5470)"

This reverts commit 957c9a9.
bradjcoughlin added a commit that referenced this pull request May 26, 2020
@bradjcoughlin
Revert "[MM-24507] Affix comment input to bottom in right-hand side (…
2891a1d 
…RHS) (#5470)"

This reverts commit d38e54e.
bradjcoughlin added a commit that referenced this pull request May 27, 2020
@bradjcoughlin
Revert "[MM-24507] Affix comment input to bottom in right-hand side (…
aae6703 
…RHS) (#5470)" (#5600)

* Revert "[MM-24507] Affix comment input to bottom in right-hand side (RHS) (#5470)"

This reverts commit 957c9a9.

* import Scrollbars, fix linting
@bradjcoughlin bradjcoughlin mentioned this pull request May 27, 2020
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@asaadmahmood asaadmahmood asaadmahmood approved these changes

@esethna esethna esethna approved these changes

@nevyangelova nevyangelova