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

[HOLD for payment 2022-10-31] [$250] Chat - Composer goes to the top on pressing expand @thesahindia #11437

Closed
kbecciv opened this issue Sep 29, 2022 · 26 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review

Comments

@kbecciv
Copy link

kbecciv commented Sep 29, 2022

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Go to staging.new.expensify.com
  2. Navigate to any chat
  3. Add few lines and press expand button at the left corner

Expected Result:

The composer should expand and shouldn't move

Actual Result:

The composer goes to the top

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • Web
  • Desktop App
  • Mobile Web/safari

Version Number: 1.2.9.0

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers): any

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Screenrecording_20220926_182030.mp4

Expensify/Expensify Issue URL:

Issue reported by: @thesahindia

Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1664196729312989

View all open jobs on GitHub

@kbecciv kbecciv added the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Sep 29, 2022
@melvin-bot
Copy link

melvin-bot bot commented Sep 29, 2022

Triggered auto assignment to @laurenreidexpensify (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@melvin-bot melvin-bot bot added Daily KSv2 and removed AutoAssignerTriage Auto assign issues for triage to an available triage team member labels Sep 29, 2022
@Puneet-here
Copy link
Contributor

Proposal

It is happening because the ReportActionCompose is wrapped by OfflineWithFeedback

<OfflineWithFeedback
pendingAction={addWorkspaceRoomPendingAction}
>
<ReportActionCompose
onSubmit={this.onSubmitComment}
reportID={reportID}
reportActions={this.props.reportActions}

To solve it we need to use style.h100 for the View at 90 and 92
We can pass styles for the first View using style but we will have to add another prop to pass style to 92 or we can just use styles.h100 or styles.flex1 because it's not breaking anything.

<View style={props.style}>
{!hideChildren && (
<View style={needsOpacity ? styles.offlineFeedback.pending : {}}>
{children}
</View>

@melvin-bot
Copy link

melvin-bot bot commented Sep 30, 2022

Triggered auto assignment to @amyevans (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@amyevans
Copy link
Contributor

Reproducible, releasing to External. A reminder @Puneet-here that you need to wait for the Help wanted label before submitting proposals, thanks! 😄

@amyevans amyevans added the External Added to denote the issue can be worked on by a contributor label Sep 30, 2022
@melvin-bot
Copy link

melvin-bot bot commented Sep 30, 2022

Current assignee @laurenreidexpensify is eligible for the External assigner, not assigning anyone new.

@melvin-bot
Copy link

melvin-bot bot commented Sep 30, 2022

Triggered auto assignment to Contributor-plus team member for initial proposal review - @rushatgabhane (External)

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 30, 2022
@melvin-bot
Copy link

melvin-bot bot commented Sep 30, 2022

Triggered auto assignment to @Beamanator (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@melvin-bot melvin-bot bot changed the title Chat - Composer goes to the top on pressing expand @thesahindia [$250] Chat - Composer goes to the top on pressing expand @thesahindia Sep 30, 2022
@amyevans amyevans removed their assignment Sep 30, 2022
@melvin-bot melvin-bot bot added the Overdue label Oct 3, 2022
@laurenreidexpensify
Copy link
Contributor

Not overdue, reviewing this morning

@melvin-bot melvin-bot bot removed the Overdue label Oct 3, 2022
@Beamanator
Copy link
Contributor

Yep ^ @rushatgabhane would you mind reviewing the one proposal in this issue today?

@laurenreidexpensify
Copy link
Contributor

@rushatgabhane
Copy link
Member

It is happening because the ReportActionCompose is wrapped by OfflineWithFeedback

@Puneet-here It is no longer the case in main. Could you please submit an updated proposal?

@rushatgabhane
Copy link
Member

rushatgabhane commented Oct 4, 2022

@Beamanator apologies for the delay, im 100% back now

@aimane-chnaif
Copy link
Contributor

Proposal

RCA:

<View style={[this.getChatFooterStyles(), this.props.isComposerFullSize && styles.chatFooterFullCompose]}>
<SwipeableView onSwipeDown={Keyboard.dismiss}>
<OfflineWithFeedback
pendingAction={this.props.addWorkspaceRoomPendingAction}
>
<ReportActionCompose
onSubmit={this.props.onSubmitComment}
reportID={this.props.report.reportID.toString()}
reportActions={this.props.reportActions}
report={this.props.report}
isComposerFullSize={this.props.isComposerFullSize}
/>
</OfflineWithFeedback>
</SwipeableView>
</View>

This happens because when composer expanded, the parent view (OfflineWithFeedback) doesn't stretch to full height so this.props.isComposerFullSize && styles.chatItemFullComposeRow in ReportActionCompose container style isn't applied so it doesn't stretch to full height.

Solution:
We need to apply same conditional style to OfflineWithFeedback as in ReportActionCompose when expanded.
OfflineWithFeedback is general component which is used throughout the app so style props should be passed

                            <OfflineWithFeedback
                                pendingAction={this.props.addWorkspaceRoomPendingAction}
+                               style={this.props.isComposerFullSize && styles.chatItemFullComposeRow}
+                               contentContainerStyle={this.props.isComposerFullSize && styles.flex1}
                            >
                                <ReportActionCompose
                                    onSubmit={this.props.onSubmitComment}
                                    reportID={this.props.report.reportID.toString()}
                                    reportActions={this.props.reportActions}
                                    report={this.props.report}
                                    isComposerFullSize={this.props.isComposerFullSize}
                                />
                            </OfflineWithFeedback>

And in https://github.com/Expensify/App/blob/main/src/components/OfflineWithFeedback.js,

        <View style={props.style}>
            {!hideChildren && (
-               <View style={needsOpacity ? styles.offlineFeedback.pending : {}}>
+               <View style={[needsOpacity ? styles.offlineFeedback.pending : {}, props.contentContainerStyle]}>
                    {children}
                </View>
            )}

@rushatgabhane
Copy link
Member

rushatgabhane commented Oct 4, 2022

@aimane-chnaif's proposal is too similar to @Puneet-here's proposal.

And @Puneet-here most likely would have submitted an updated proposal same as @aimane-chnaif's proposal

@rushatgabhane
Copy link
Member

@Beamanator I think we should hire @Puneet-here because they were first.

C+ reviewed 🎀 👀 🎀

@Beamanator
Copy link
Contributor

Makes sense to me @rushatgabhane

@melvin-bot melvin-bot bot added the Weekly KSv2 label Oct 4, 2022
@melvin-bot

This comment was marked as outdated.

@aimane-chnaif
Copy link
Contributor

@Puneet-here's proposal didn't explain that those styles needed only when composer expanded (this.props.isComposerFullSize === true).
WIthout this condition, it works on web but breaks style on mobile.
Since this is important flag, I came to submit proposal though already proposal exists

@rushatgabhane
Copy link
Member

rushatgabhane commented Oct 4, 2022

Agree with @aimane-chnaif, @Beamanator sorry for the back and forth but I think we should hire @aimane-chnaif

@Beamanator
Copy link
Contributor

Ooh thanks for bringing that up @aimane-chnaif , this one is tough since the proposal are both small, but I agree with @rushatgabhane let's change up who was hired 👍

@melvin-bot
Copy link

melvin-bot bot commented Oct 5, 2022

📣 @aimane-chnaif You have been assigned to this job by @Beamanator!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@aimane-chnaif
Copy link
Contributor

Thanks @rushatgabhane @Beamanator
PR is ready for review

@melvin-bot melvin-bot bot added the Overdue label Oct 14, 2022
@Beamanator
Copy link
Contributor

PR under review, should be merged soon 👍

@melvin-bot melvin-bot bot removed the Overdue label Oct 14, 2022
@laurenreidexpensify laurenreidexpensify added the Reviewing Has a PR in review label Oct 16, 2022
@puneetlath puneetlath added the Bug Something is broken. Auto assigns a BugZero manager. label Oct 19, 2022
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Oct 24, 2022
@melvin-bot melvin-bot bot changed the title [$250] Chat - Composer goes to the top on pressing expand @thesahindia [HOLD for payment 2022-10-31] [$250] Chat - Composer goes to the top on pressing expand @thesahindia Oct 24, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 24, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.18-10 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2022-10-31. 🎊

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 Daily KSv2 labels Oct 31, 2022
@laurenreidexpensify
Copy link
Contributor

laurenreidexpensify commented Oct 31, 2022

Payments:

@rushatgabhane @aimane-chnaif as soon as you've accepted job in Upwork I will issue payment. Thanks

@laurenreidexpensify
Copy link
Contributor

Everyone has been paid 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

8 participants