MM-22757 Fix for date toast placement in web mobile view #5090

sudheerDev · 2020-03-19T22:11:51Z

Summary

Push the toast by 50px when toast is present

Issue: FloatingTimestamp component need to know if any toasts are present in the UI. toasts state is only available in ToastWrapper as of now.

Solutions:

Either use DOM manipulation like we do in few places of codebase and add a class when toast is present with add.classList
Add a redux state to keep track of toast and use a selector
Use something like ReactClone and get the required prop from ToastWrapper.
We could have FloatingTimestamp in ToastWrapper

i went with 3 reasons being

Really hacky and msot places we use in codebase tend to fail and cause regressions
Feel like too much of work for such a small use case
need to add couple more props to ToastWrapper so it can be passed to FloatingTimestamp

Ticket Link

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

* Check if toast is present and push it down by 50px

sudheerDev · 2020-03-19T22:16:06Z

@nevyangelova @Willyfrog Let me know what you guys think. I am going to add tests after you guys have a look at it.

nevyangelova · 2020-03-20T11:33:53Z

components/post_view/floating_timestamp/floating_timestamp.jsx

 className += ' rhs';
 }

 return (
 <div
 className={className}
- style={this.props.stylesOverride}
+ style={toastPresent ? {top: '50px'} : null}


0/5 maybe we can drive this by className.

i am not a fan of using classes if they are not reusable. It is hard to clean up with the amount of css we already have. I am gonna change to class though.

nevyangelova · 2020-03-20T11:36:32Z

components/toast_wrapper/toast_wrapper.jsx

 unreadToastProps = {
 ...unreadToastProps,
 onDismiss: this.hideUnreadToast,
 onClick: this.scrollToLatestMessages,
 onClickMessage: Utils.localizeMessage('postlist.toast.scrollToBottom', 'Jump to recents'),
 show: true,
- showActions: !this.props.atLatestPost || (this.props.atLatestPost && !this.props.atBottom),
+ showActions: !atLatestPost || (atLatestPost && !atBottom),


1/5 can be simplified to !(atLatestPost && atBottom)

Aah, Nice. i cannot wrap my head around conditions usually. Will change

nevyangelova

Looks good though I like the solution 👍

Willyfrog

So I think I’d have gone for #2 as it would allow for further usage if needed, not only for this view but for any other we might have. Having a way to know if the toast is present and probably some info with it, might be an easy way to share all the calculations already done for displaying on the toast.

With that said, this is an interesting approach, I didn’t know about react.clone, so that’s a new thing, but if it will always depends on the toast to be rendered, shouldn’t we pass the needed info to have the toast render it and remove cloning? Or are we planning to reuse the toast on other situations?

Willyfrog · 2020-03-20T11:02:31Z

components/post_view/floating_timestamp/floating_timestamp.jsx

 className += ' rhs';
 }

 return (
 <div
 className={className}
- style={this.props.stylesOverride}
+ style={toastPresent ? {top: '50px'} : null}


Why don’t we use classname for this? We are already adding classes depending on what is around the floating timestamp, so another if wouldn’t hurt and it would probably be cleaner.

Classes already exist and there weren't part of the PR. As i said i am not a fan personally to use classes in out codebase anymore for things which are not reusable, or at least applied unconditionally. Things like these tend to make maintenance of css hard. There were some arguments in the past about performance of css vs inline styles in react but with the level of nesting we normally create the browsers will have a much harder time figuring out the specificity IMO.

In this case i am gonna change to class as it is popular behaviour and i am like 1/5 on this.

sudheerDev · 2020-03-20T12:30:21Z

So I think I’d have gone for #2 as it would allow for further usage if needed, not only for this view but for any other we might have. Having a way to know if the toast is present and probably some info with it, might be an easy way to share all the calculations already done for displaying on the toast.

Ya, i think i am gonna change it to have a state in redux.

With that said, this is an interesting approach, I didn’t know about react.clone, so that’s a new thing, but if it will always depends on the toast to be rendered, shouldn’t we pass the needed info to have the toast render it and remove cloning? Or are we planning to reuse the toast on other situations?

The problem removing the cloning and passing in the props is that we need isMobile prop which means i need to add a state and a listener again in this component or pass that to ToastWrapper which only needs because of the FloatingTimestamp. Seemed like it has its own downsides.

sudheerDev · 2020-03-24T08:41:13Z

@Willyfrog @nevyangelova this is up for review.

Willyfrog

lgtm, slight concern about naming but nothing too serious

Willyfrog · 2020-03-24T14:46:34Z

actions/views/channel.test.js

+ data: true,
+ type: 'UPDATE_TOAST_STATUS'


slightly concerned about the naming here: what should someone think of "update_toast_status=true"? maybe we should rename into something more descriptive?

i am 0/5 on this but this is not something that people stumble up on when reading code IMO.

components/post_view/floating_timestamp/floating_timestamp.jsx

sudheerDev · 2020-03-25T09:57:06Z

/update-branch

mattermod · 2020-03-25T09:57:08Z

Error trying to update the PR.
Please do it manually.

mattermod · 2020-03-25T14:45:48Z

Mattermost test server updated with git commit 7512c38861839dacd77bc752c55ae9f18e3b8819.

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

jgilliam17

Thank you @sudheerDev
Tested, looks good to merge.

Verified date toast appears below unread/new message toast in mobile web view
There is an issue with the history toast missing, and will be filed separately

mattermod · 2020-03-25T18:38:58Z

Test server destroyed

mattermod · 2020-03-25T18:39:20Z

@sudheerDev
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-#5090-upstream-release-5.22-1585161559
Branch 'automated-cherry-pick-of-#5090-upstream-release-5.22-1585161559' set up to track remote branch 'release-5.22' from 'upstream'.
+++ Downloading patch to /tmp/5090.patch (in case you need to do this again)

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

Applying: MM-22757 Fix for date toast placement in web mobile view
Using index info to reconstruct a base tree...
M	components/post_view/post_list_virtualized/post_list_virtualized.jsx
M	components/toast_wrapper/toast_wrapper.jsx
Falling back to patching base and 3-way merge...
Auto-merging components/toast_wrapper/toast_wrapper.jsx
CONFLICT (content): Merge conflict in components/toast_wrapper/toast_wrapper.jsx
Auto-merging components/post_view/post_list_virtualized/post_list_virtualized.jsx
Patch failed at 0001 MM-22757 Fix for date toast placement in web mobile view
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/toast_wrapper/toast_wrapper.jsx

+++ Aborting in-progress git am.

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

* MM-22757 Fix for date toast placement in web mobile view * Check if toast is present and push it down by 50px * Rename func * Change to dispatching an action on change of toast * * Use className utility * Fix for lint * Fix tests after master merge * Change default value in reducer

jgilliam17 · 2020-03-25T19:55:38Z

Please disregard all the label changes and a review re-request.
I misunderstood that history toast fix will be included in this PR and changed a bunch labels 😞

sudheerDev added 2 commits March 20, 2020 03:22

MM-22757 Fix for date toast placement in web mobile view

f14abbc

* Check if toast is present and push it down by 50px

Rename func

fb14225

sudheerDev added the Work in Progress Not yet ready for review label Mar 19, 2020

sudheerDev added this to the v5.22.0 milestone Mar 19, 2020

sudheerDev requested review from Willyfrog and nevyangelova March 19, 2020 22:11

nevyangelova reviewed Mar 20, 2020

View reviewed changes

nevyangelova approved these changes Mar 20, 2020

View reviewed changes

sudheerDev requested a review from nevyangelova March 20, 2020 11:41

Willyfrog reviewed Mar 20, 2020

View reviewed changes

Change to dispatching an action on change of toast

d5537f8

sudheerDev requested a review from Willyfrog March 20, 2020 19:45

sudheerDev added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester CherryPick/Approved Meant for the quality or patch release tracked in the milestone and removed Work in Progress Not yet ready for review labels Mar 20, 2020

Willyfrog approved these changes Mar 24, 2020

View reviewed changes

nevyangelova reviewed Mar 25, 2020

View reviewed changes

components/post_view/floating_timestamp/floating_timestamp.jsx Outdated Show resolved Hide resolved

components/post_view/floating_timestamp/floating_timestamp.jsx Outdated Show resolved Hide resolved

* Use className utility

399c36d

sudheerDev added 3 commits March 25, 2020 15:31

Merge branch 'master' into MM-22757

aeeb399

Fix for lint

c946840

Fix tests after master merge

dfe3d73

sudheerDev requested a review from nevyangelova March 25, 2020 11:48

nevyangelova approved these changes Mar 25, 2020

View reviewed changes

sudheerDev added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Mar 25, 2020

sudheerDev requested a review from jgilliam17 March 25, 2020 11:49

sudheerDev added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Mar 25, 2020

Change default value in reducer

7512c38

jgilliam17 approved these changes Mar 25, 2020

View reviewed changes

jgilliam17 added QA Review Done and removed 3: QA Review Requires review by a QA tester Setup Cloud Test Server Setup a test server using Mattermost Cloud labels Mar 25, 2020

sudheerDev merged commit ff16e0e into mattermost:master Mar 25, 2020

sudheerDev deleted the MM-22757 branch March 25, 2020 18:41

sudheerDev 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 Mar 25, 2020

jgilliam17 self-requested a review March 25, 2020 18:54

jgilliam17 added QA Review Done and removed QA Review Done labels Mar 25, 2020

amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Mar 26, 2020

jgilliam17 added the Tests/Done Release tests have been written label Apr 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MM-22757 Fix for date toast placement in web mobile view #5090

MM-22757 Fix for date toast placement in web mobile view #5090

sudheerDev commented Mar 19, 2020

sudheerDev commented Mar 19, 2020

nevyangelova Mar 20, 2020

sudheerDev Mar 20, 2020

nevyangelova Mar 20, 2020

sudheerDev Mar 20, 2020

nevyangelova left a comment •

edited

Loading

Willyfrog left a comment

Willyfrog Mar 20, 2020

sudheerDev Mar 20, 2020

sudheerDev commented Mar 20, 2020 •

edited

Loading

sudheerDev commented Mar 24, 2020

Willyfrog left a comment

Willyfrog Mar 24, 2020

sudheerDev Mar 24, 2020

sudheerDev commented Mar 25, 2020

mattermod commented Mar 25, 2020

mattermod commented Mar 25, 2020

jgilliam17 left a comment

mattermod commented Mar 25, 2020

mattermod commented Mar 25, 2020

jgilliam17 commented Mar 25, 2020

MM-22757 Fix for date toast placement in web mobile view #5090

MM-22757 Fix for date toast placement in web mobile view #5090

Conversation

sudheerDev commented Mar 19, 2020

Summary

Ticket Link

sudheerDev commented Mar 19, 2020

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

nevyangelova left a comment • edited Loading

Choose a reason for hiding this comment

Willyfrog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sudheerDev commented Mar 20, 2020 • edited Loading

sudheerDev commented Mar 24, 2020

Willyfrog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sudheerDev commented Mar 25, 2020

mattermod commented Mar 25, 2020

mattermod commented Mar 25, 2020

jgilliam17 left a comment

Choose a reason for hiding this comment

mattermod commented Mar 25, 2020

mattermod commented Mar 25, 2020

jgilliam17 commented Mar 25, 2020

nevyangelova left a comment •

edited

Loading

sudheerDev commented Mar 20, 2020 •

edited

Loading