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

[$250] BUG: when flipping between chats, the composer box is gone and only the skeleton UI shows. #11962

Closed
kavimuru opened this issue Oct 18, 2022 · 23 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@kavimuru
Copy link

kavimuru commented Oct 18, 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 ND and login
  2. Open any chat
  3. Switch between chats

Expected Result:

Composer window should not disappear

Actual Result:

composer window disappears between chats and the UI jumping around.

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android

Version Number: 1.2.17-1
Reproducible in staging?: y
Reproducible in production?: y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:
image

Recording.716.mp4

Expensify/Expensify Issue URL:
Issue reported by: @shawnborton
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1666030320048069

View all open jobs on GitHub

@kavimuru kavimuru added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Oct 18, 2022
@tjferriss tjferriss removed their assignment Oct 19, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 19, 2022

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

@mountiny mountiny self-assigned this Oct 19, 2022
@mountiny mountiny removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Oct 19, 2022
@mountiny mountiny added the External Added to denote the issue can be worked on by a contributor label Oct 19, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 19, 2022

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

@melvin-bot
Copy link

melvin-bot bot commented Oct 19, 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 Oct 19, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 19, 2022

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

@melvin-bot melvin-bot bot changed the title BUG: when flipping between chats, the composer box is gone and only the skeleton UI shows. [$250] BUG: when flipping between chats, the composer box is gone and only the skeleton UI shows. Oct 19, 2022
@mountiny
Copy link
Contributor

This can definitely be external issue, the UI jumps are not great, lets make sure it is spotless

@puneetlath puneetlath added the Bug Something is broken. Auto assigns a BugZero manager. label Oct 19, 2022
@MitchExpensify
Copy link
Contributor

Upwork Job

@chauchausoup
Copy link

chauchausoup commented Oct 20, 2022

Proposal:

Issue:
Issue is caused due to ReportFooter component being wrapped along with the ReportActionsView here at:

<ReportFooter

that is rendered by a conditional logic to check the Loading effect in the app.

Solution:
We can simple take the ReportFooter component and keep it outside the conditional wrapper as:

From:

         {this.shouldShowLoader()
                                ? (
                                    <ReportActionsSkeletonView
                                        containerHeight={this.state.skeletonViewContainerHeight}
                                    />
                                )
                                : (
                                    <>
                                        <ReportActionsView
                                            reportActions={this.props.reportActions}
                                            report={this.props.report}
                                            session={this.props.session}
                                            isComposerFullSize={this.props.isComposerFullSize}
                                            isDrawerOpen={this.props.isDrawerOpen}
                                        />
                                        <ReportFooter
                                            addWorkspaceRoomErrors={addWorkspaceRoomErrors}
                                            addWorkspaceRoomPendingAction={addWorkspaceRoomPendingAction}
                                            isOffline={this.props.network.isOffline}
                                            reportActions={this.props.reportActions}
                                            report={this.props.report}
                                            isComposerFullSize={this.props.isComposerFullSize}
                                            onSubmitComment={this.onSubmitComment}
                                        />
                                    </>
                                )}

To:

{ this.shouldShowLoader()
                                ? (
                                    <ReportActionsSkeletonView
                                        containerHeight={this.state.skeletonViewContainerHeight}
                                    />
                                )
                                : (
                                    <>
                                        <ReportActionsView
                                            reportActions={this.props.reportActions}
                                            report={this.props.report}
                                            session={this.props.session}
                                            isComposerFullSize={this.props.isComposerFullSize}
                                            isDrawerOpen={this.props.isDrawerOpen}
                                        />

                                    </>
                                )}
                            <ReportFooter
                                addWorkspaceRoomErrors={addWorkspaceRoomErrors}
                                addWorkspaceRoomPendingAction={addWorkspaceRoomPendingAction}
                                isOffline={this.props.network.isOffline}
                                reportActions={this.props.reportActions}
                                report={this.props.report}
                                isComposerFullSize={this.props.isComposerFullSize}
                                onSubmitComment={this.onSubmitComment}
                            />
                            

@rushatgabhane
Copy link
Member

@chauchausoup nice and simple! i like that you've mentioned the cause as well.

@rushatgabhane
Copy link
Member

rushatgabhane commented Oct 20, 2022

@mountiny let's go ahead with @chauchausoup's proposal

🎀 👀 🎀 C+ reviewed

@mountiny mountiny added Weekly KSv2 and removed Daily KSv2 labels Oct 20, 2022
@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 20, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 20, 2022

📣 @chauchausoup You have been assigned to this job by @mountiny!
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 📖

@mountiny
Copy link
Contributor

Thank you for the review @rushatgabhane, can you please raise a PR @chauchausoup ?

@chauchausoup
Copy link

Sure @mountiny will spin it in few hours.

@mountiny
Copy link
Contributor

PR is in a review.

@0xmiros
Copy link
Contributor

0xmiros commented Oct 31, 2022

This is duplicated with #11856.
Accepted proposal here simply reverts #11853 which fixes #11840 so it causes regression.
@rushatgabhane you're also C+ on #11856 so please check my proposal.

@mountiny
Copy link
Contributor

@rushatgabhane Can you confirm if the regression would be avoided in here? Thank you for looking into this 🙇

@rushatgabhane
Copy link
Member

@mountiny PR #11840 would indeed cause the regression.

@mountiny
Copy link
Contributor

mountiny commented Nov 4, 2022

😬 Thank you for confirming @rushatgabhane and thanks for pointing this out @0xmiroslav

@chauchausoup can you think of a way how to prevent the regression from happening and solving this issue? Otherwise I am afraid we will have to open this for new proposals.

@rushatgabhane
Copy link
Member

@mountiny @0xmiroslav's proposal would fix the issue and not cause any regression :)

@rushatgabhane
Copy link
Member

rushatgabhane commented Nov 4, 2022

So a way to move forward could be close this issue and the linked PR in favor of #11856

@mountiny
Copy link
Contributor

mountiny commented Nov 4, 2022

Oh perfect, @rushatgabhane, thanks!

I think we can close this one then, since it is duplicate and the other issue has higher reward.

I am sorry @chauchausoup this did not work out, thank you so much for your efforts but we cannot proceed with that proposal given it would introduce regression.

@mountiny mountiny closed this as completed Nov 4, 2022
@chauchausoup
Copy link

I dint understand whats happening. How do it cause regression when my proposal is not even deployed?

@chauchausoup
Copy link

My proposal was posted earlier than @0xmiroslav 's right?

@mountiny
Copy link
Contributor

mountiny commented Nov 5, 2022

@chauchausoup sorry for the confusion. You can read this comment, the solution you have proposed is a revert of a PR which fixed a regression.

Rushat has confirmed that your PR is experiencing the issue so it would cause a regression unfortunately. I realize you have proposed solution earlier however, since this solution has a bug, the next proposal which avoids it goes forward.

Apologies for this.

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

No branches or pull requests

9 participants