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

IOU - mWeb - Page looks broken after click on Comment section #3354

Closed
isagoico opened this issue Jun 3, 2021 · 27 comments
Closed

IOU - mWeb - Page looks broken after click on Comment section #3354

isagoico opened this issue Jun 3, 2021 · 27 comments
Assignees
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@isagoico
Copy link

isagoico commented Jun 3, 2021

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


Expected Result:

Page looks good after click on Comment section

Actual Result:

Page looks broken after click on Comment section

Action Performed:

  1. https://staging.expensify.cash
  2. Log in with applause account
  3. Click on Split Money
  4. Add users with who you want to split money
  5. Click Next
  6. Put comments in Comment section

Workaround:

Visual issue.

Platform:

Where is this issue occurring?

Web
iOS
Android
Desktop App
Mobile Web ✔️

Version Number: 1.0.62-0

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

Notes/Photos/Videos:

Expensify/Expensify Issue URL:

View all open jobs on Upwork

Job post - https://www.upwork.com/jobs/~01699bc381521d62ca

@isagoico isagoico added DeployBlockerCash This issue or pull request should block deployment Engineering Daily KSv2 labels Jun 3, 2021
@MelvinBot
Copy link

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

@OSBotify
Copy link
Contributor

OSBotify commented Jun 3, 2021

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@github-actions github-actions bot added Hourly KSv2 and removed Daily KSv2 labels Jun 3, 2021
@isagoico isagoico removed the DeployBlockerCash This issue or pull request should block deployment label Jun 3, 2021
@isagoico
Copy link
Author

isagoico commented Jun 3, 2021

Accidentally added the deploy blocker label, removing.

@Jag96 Jag96 added Daily KSv2 and removed Hourly KSv2 labels Jun 4, 2021
@isagoico
Copy link
Author

isagoico commented Jun 7, 2021

Issue reproducible today during KI retests

@Beamanator
Copy link
Contributor

Beamanator commented Jun 7, 2021

Hmm from my testing, it seems to be ok on iOS mWeb, but not on Android mWeb:

iOS mWeb: Android mWeb:
Screen Shot 2021-06-07 at 11 35 16 AM WhatsApp Image 2021-06-07 at 11 40 11 AM

@Beamanator
Copy link
Contributor

I believe this is able to be fixed externally, so adding External 👍

@Beamanator Beamanator added the External Added to denote the issue can be worked on by a contributor label Jun 7, 2021
@MelvinBot
Copy link

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

@kevinksullivan
Copy link
Contributor

Job posted https://www.upwork.com/jobs/~01699bc381521d62ca

@MelvinBot
Copy link

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

@Beamanator
Copy link
Contributor

Beamanator commented Jun 9, 2021

@timszot unassigning you since I'm happy to review proposals for this one & I'm already assigned

@trjExpensify
Copy link
Contributor

trjExpensify commented Jun 9, 2021

Presuming the fix will cover both request money and split bill flows, but just popping a note here that it occurs on the IOUConfirm page on mWeb for both, so let's make sure the tests in the PR cover both. 👍

@parasharrajat
Copy link
Member

parasharrajat commented Jun 9, 2021

@Beamanator I would like to fix this.

Proposal

So to fix this we need to update the UI of https://github.com/Expensify/Expensify.cash/blob/cf45c51d69405be841a6967769a9cbcd69ab2bfc/src/components/IOUConfirmationList.js.

  1. We will wrap our participant list in ScrollView, then move the bottom button out of the list
    like
<>
  <ScrollView style={[styles.flex1, styles.w100]}>
                    <OptionsList
                        listContainerStyles={[{
                            // Give max height to the list container so that it does not extend
                            // beyond the comment view as well as button
                            maxHeight: this.props.windowHeight - MINIMUM_BOTTOM_OFFSET
                                - this.props.insets.top - this.props.insets.bottom,
                        }]}
                        sections={this.getSections()}
                        disableArrowKeysActions
                        disableRowInteractivity
                        hideAdditionalOptionStates
                        forceTextUnreadStyle
                        canSelectMultipleOptions={this.props.hasMultipleParticipants}
                        disableFocusOptions
                        selectedOptions={this.getAllOptionsAsSelected()}
                    />

                        <Text style={[styles.p5, styles.textMicroBold, styles.colorHeading]}>
                            {this.props.translate('iOUConfirmationList.whatsItFor')}
                        </Text>
                    <View style={[styles.ph5]}>
                        <TextInput
                            style={[styles.textInput]}
                            value={this.props.comment}
                            onChangeText={this.props.onUpdateComment}
                            placeholder={this.props.translate('common.optional')}
                            placeholderTextColor={themeColors.placeholderText}
                        />
                    </View>
            </ScrollView>
                      <View style={[styles.ph5, styles.pb3]}>
                      <Button
                          success
                          style={[styles.mb2]}
                          isLoading={this.props.iou.loading}
                          text={buttonText}
                          onPress={() => this.props.onConfirm(this.getSplits())}
                      />
                  </View>
                  </>

It's hard to explain but a video tells the whole story here.

1623244945758.mp4

This will cover both request money and split bill flows.

@Beamanator
Copy link
Contributor

Beamanator commented Jun 10, 2021

@parasharrajat Hmm I guess I'd like the @Expensify/design team's thoughts on this one. My thought was that the "What's it for" text + the TextInput + the Split / Request button should all stay together. So when the keyboard opens or if you scroll the list of users, those 3 elements are always visible above the keyboard.

@parasharrajat
Copy link
Member

Oh. Yeah . Let's wait from the design team to confirm this and I can rework my proposal.

@trjExpensify
Copy link
Contributor

So when the keyboard opens or if you scroll the list of users, those 3 elements are always visible above the keyboard.

I think the intention is that they should, as per this issue to make the button styles consistent. CC: @NikkiWines

@NikkiWines
Copy link
Contributor

NikkiWines commented Jun 10, 2021

Yeah the button style + whether or not it's above the keyboard should be consistent but I agree let's defer to @Expensify/design for this and see if we need to make tweaks.

@isagoico
Copy link
Author

Issue reproducible today during KI retests

@trjExpensify
Copy link
Contributor

👋 @michelle-thompson @megankelso @shawnborton can we get your thoughts on this one please, so we can move forward with a proposal. Thanks!

@shawnborton
Copy link
Contributor

I agree with @parasharrajat 's proposal in that the bottom green button should always be fixed to the bottom of the viewport. When the keyboard comes in, the bottom of the viewport sits above the keyboard. Basically the video looks great to me, though perhaps we want to add a little bit of padding below the last input (What's it for?) so that there can be some visual space between the input and button?

@Beamanator
Copy link
Contributor

So just to be clear @shawnborton , you like the fact that the "what's it for" label & text input can be scrolled down below / behind the "Split" button?

@shawnborton
Copy link
Contributor

I wouldn't say so much that I like it, but I don't mind it :) It mostly just keeps the button behavior consistent with other pages where the primary button is always docked to the bottom of the view.

@Beamanator
Copy link
Contributor

Haha makes sense to me :D

@parasharrajat please go ahead and submit a PR for the fix when you have a chance! 👍 Note the tiny style adjustment:

perhaps we want to add a little bit of padding below the last input (What's it for?) so that there can be some visual space between the input and button?

@isagoico
Copy link
Author

Issue not reproducible today during KI retests. (First week)

@parasharrajat
Copy link
Member

Hi, @Beamanator I forgot to tag this issue PR in my Button Styling PR #3193. It was fixed there. So this issue can be closed. But on M-web Safari, the button is not fixed at the bottom due to the reason that Safari keeps the focused input in the middle of the screen and doesn't respect Flex in the case. I am looking to find a better way for that. It is looking difficult to keep the app intact for all the platforms and still satisfy Safari Mobile.

@parasharrajat
Copy link
Member

Hi @Beamanator. any thoughts? Should we close this issue as this has been solved in #3193. Please check the last two checklist items in detail.

@Beamanator
Copy link
Contributor

@parasharrajat I just tested locally, and you're right - We can close this out 👍

Here's my tests on Android, mWeb showing the fix working:

Screen.Recording.2021-06-24.at.3.03.14.PM.mov

@kevinksullivan
Copy link
Contributor

@parasharrajat just sent you an offer for this job. Once you accept I'll pay it in Upwork!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests