Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
2.99k stars 2.5k forks source link

[$250] IOS - Submit expense - Next button not fully visible after returning online #41810

Open lanitochka17 opened 1 week ago

lanitochka17 commented 1 week ago

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


Version Number: 1.4.71-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Issue reported by: Applause - Internal Team

Action Performed:

  1. Open the app and log in
  2. Disable internet connection
  3. Tap FAB > Submit expense
  4. Select any tab, e.g. Distance (the same for manual and scan request)
  5. Return online

Expected Result:

The Next button is fully visible

Actual Result:

Only the top of the Next button is visible. It is reproducible with Manual and Scan tabs

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/1569cd40-1bf8-4c99-89db-5d217dfe1c36

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01f1a3e2d4eb573fb6
  • Upwork Job ID: 1788618114597507072
  • Last Price Increase: 2024-05-16
Issue OwnerCurrent Issue Owner: @eVoloshchak
melvin-bot[bot] commented 1 week ago

Triggered auto assignment to @sonialiap (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

lanitochka17 commented 1 week ago

@sonialiap FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

lanitochka17 commented 1 week ago

We think that this bug might be related to #wave-collect - Release 1

neonbhai commented 1 week ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Next button not fully visible after returning online

What is the root cause of that problem?

StepScreenWrapper by default adds safeAreaPaddingBottom that overlaps the bottom button in request pages

What changes do you think we should make in order to solve the problem?

We will pass includeSafeAreaPaddingBottom={false} to StepScreenWrapper in affected pages in IOU request steps.

melvin-bot[bot] commented 1 week ago

Job added to Upwork: https://www.upwork.com/jobs/~01f1a3e2d4eb573fb6

melvin-bot[bot] commented 1 week ago

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

samilabud commented 1 week ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Native small screens - Submit expense - Next button not fully visible after returning online

What is the root cause of that problem?

Currently when we are using "small screens" (or native screens) the MapView try to use the space available to render itself, this space is calculated by the ScreenWrapper component based in the initialHeight (in this case) and the children components.

The issue is for native screens when we render the map the first time (when online) we take the all available space, but then when we go offline we show/render an offline indicator, this makes the available height reduced (so we see the map pending indicator a bit short than the map), then when we go online the map try to recover its original height (but now we have less height space due the offline indicators) and move down the next button.

What changes do you think we should make in order to solve the problem?

We should determinate in IOURequestStepDistance if we are using native small screens (no web) and if so we going to use flexbox to fix the height of the map based at least 3 points (grow relative to the rest of the flex) and the button container less than 1 (specific 90%) of grow relative like:

    const {isSmallScreen} = useWindowDimensions();
    const isIos = Platform.OS === 'ios';
    const isNotWeb = Platform.OS !== 'web';
    const isSmallScreenIOSNoWeb = isIos && isNotWeb && isSmallScreen;
<View style={styles.flex1}>
                <View style={isSmallScreenIOSNoWeb ? styles.flex3 : styles.flex1}>
                    <DraggableList
                        data={waypointsList}
                        keyExtractor={(item) => item}
                        shouldUsePortal
                        onDragEnd={updateWaypoints}
                        ref={scrollViewRef}
                        renderItem={renderItem}
                        ListFooterComponent={
                            <DistanceRequestFooter
                                waypoints={waypoints}
                                navigateToWaypointEditPage={navigateToWaypointEditPage}
                                transaction={transaction}
                            />
                        }
                    />
                </View>
               <View style={isSmallScreenIOSNoWeb ? [{flex: 0.9}] : [styles.w100, styles.pt2]}>
.
.
.
Tests: Before changes: https://github.com/Expensify/App/assets/5216036/a5e17dce-7de6-41d4-9367-7d68097e55c8 After changes MAC Chrome: https://github.com/Expensify/App/assets/5216036/263d7989-ddc5-4366-b1eb-27ceab7daf5b Android Chrome and Iphone Safari: https://github.com/Expensify/App/assets/5216036/7bb216d4-a159-4aa7-9d92-0cbe4fc23022 Android and Iphone (native) https://github.com/Expensify/App/assets/5216036/1c1b0696-10c5-45dd-9047-7f6ad115e26f
melvin-bot[bot] commented 5 days ago

@eVoloshchak, @sonialiap Huh... This is 4 days overdue. Who can take care of this?

sonialiap commented 4 days ago

@eVoloshchak what do you think of the above proposal?

melvin-bot[bot] commented 3 days ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 3 days ago

@eVoloshchak, @sonialiap 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

eVoloshchak commented 2 days ago

@neonbhai, I've applied your proposal to IOURequestStepDistance, but it doesn't resolve the issue, could you re-test this please?

@samilabud, thank you for the thorough explanation!

We should determinate in IOURequestStepDistance if we are using small screens and if so calculate the height of the map based in percent of the screen height resting the height of the offline indicator to guarantee the space height when the map hide/show (re-renders)

Can we make the map view fill all the available space? The problem with calculating its height manually is it's prone to breakage in the future. If we change the font size, number of lines, height of offline indicator, or add another button below the map view - the calculation will become wrong

samilabud commented 2 days ago

The problem with calculating its height manually

Good point, I will try to do a formula to calculate the height based to the current calculation that we do in ScreenWrapper, this way I think this going to persist good in the future.

samilabud commented 1 day ago

Can we make the map view fill all the available space?

At the end the only way I found was using flexbox, I have updated my proposal, please check it out 🙏🏼:

Updated