[$1000] [HOLD for payment 2023-07-20] [Manual Requests] Make the Datepicker component more reusable

[mountiny]

Problem

The NewDatePicker component has went trhough couple of iterations and its time to refactor it and mainly make it easier to reuse in other flows and pages of the app. Some context in this thread

Why is it important?

We are adding more flows which require a date picker component, however, the current implementation is hard to just take and plug into some other form with different routes so it just works. For example in this PR we need to make it easy to implement a date picker to a request details page

Solution

Explore how we can simplify this component so we keep the current functionality and design ideally while improving the DX.

cc @Beamanator @burczu @WoLewicki @adamgrzybowski @s77rt @luacmartins

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~018d352aca1b0ce802
• Upwork Job ID: 1683554983031660544
• Last Price Increase: 2023-07-24

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Bug0 Triage Checklist (Main S/O)

• [ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• [ ] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• [ ] This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• [ ] This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• [ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[mountiny]

Asking Bartek if he wants to tackle this one tomorrow as they originally worked on this issue

[burczu]

Hi I'm Bartek from Callstack - expert contributor group - I would like to work on this issue.

[WoLewicki]

I would gladly review the part with subscribing to navigation and the information flow in those components 😅

[melvin-bot[bot]]

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

[mountiny]

Nice, I will assign you too, its important issue to get done right

[mountiny]

@burczu previously, we wanted the Year page to be its own route with the psuh-to-page design which is quite complicated.

Could we potentially make this page "internal" to the datepicker route so it looks same as a new page in the stack. This might introduce issues with the navigation tho to keep the browser back and in-app Up actions consistent.

Maybe we could also disallow deeplinking to the year page which could help with some edge cases so that when you try to access the year page by using deeplink it would redirect to the date picker first.

Not sure whats the best way but lets brainstorm our options

[burczu]

not overdue

[ArekChr]

In my opinion, it's a great decision to eliminate the persistence of the calendar state upon reloading. The current implementation relies on having a separate route for the list of years in the calendar because it uses navigation. However, in most common websites that utilize a calendar picker, there is no state persistence. Removing it would make the code more reusable and much easier to maintain

[mountiny]

Thanks for chiming in @ArekChr. I agree this will make the Datepicker better component from the developer perspective and there is questionable benefit from having the state persistance. Its in theory nice for users that the state is not lost but we dont really have any data or proves users would be running to this issue, they will most likely "get it".

[mountiny]

Would you be able to prepare a proposal for the refactor keeping the same open and close animation for the year picker?

[ArekChr]

Yes @mountiny, I will work on that.

[ArekChr]

Proposal

Problem Statement

The New Date Picker component has gone through several iterations, and refactoring is necessary to improve reusability across various flows and pages within the application.

Root Cause

The main issue lies in implementing the year list picker as a separate screen using react-navigation. This approach introduces additional complexity when navigating between the calendar and the year picker, resulting in various problems and regressions.

Proposed Solution

To address the problem effectively, I propose the following changes:

1. Refactor the push-to-page screen to be a view within the calendar component. Integrating the year list picker directly into the calendar can eliminate complex navigation logic and simplify the overall implementation. This step includes:

• Removing all related navigation logic in YearPickerPage and value selectedYear that comes from route props in DateOfBirthPage
• Implementing a new logic to pass states between the list and calendar through props. Since navigation is removed, the following props in CalendarPicker will be removed: selectedMonth, selectedYear and onYearPickerOpen function. These props will be handled inside the CalendarPicker. Managing the state through props simplifies the use case, improves readability, and provides better testing possibilities for the year list picker.
• Implementing an animation replicating the push-to-page screen's behaviour using reanimated. This ensures a seamless transition between the calendar and the year list, enhancing the overall usability of the component.

2. Refactor the folder structure to improve component readability. Move the CalendarPicker folder into the NewDatePicker folder to ensure better organization and eliminate unused components.
3. Refactor prop types as suggested in the discussion https://github.com/Expensify/App/pull/15343#discussion_r1162163822. Move all prop types from the new date picker to its component since they are not exported elsewhere.
4. Improve test coverage by utilizing the react-native-testing-library (RNTL). Add specific tests for selecting a year using the year picker within the calendar. This will facilitate easier testing of the year picker functionality and increase confidence in its behaviour.
5. Introduce regression tests using RNTL and reassure to measure render counts and execution time when selecting a year and month and switching between months using arrow buttons. Monitoring these metrics will ensure optimal component performance and help identify potential regressions.

Implementing these proposed changes will enhance the reusability, maintainability, and overall quality of the NewDatePicker component. It will simplify the implementation, improve user experience, and increase test coverage, resulting in a more robust and reliable solution.

[mountiny]

@ArekChr Thanks for the proposal, I think the plan looks good. I think the regression tests might not be that necessary at first they can come in a separate PRs for sure.

Lets ensure the UI of the page and animation using reanimated looks same as other page transitions.

Additionally I will ping @JmillsExpensify and @shawnborton here as they have been part of the previous discussions about the NewDatePicker component.

Do you have any concerns about the Year selector not being its own page anymore? It will simplify the code drastically, make it easy to reuse and understand which we both lack now. We doubt that the UX will be degraded. In case of Expense created date, users rarely even have to change year of the created date.

[ArekChr]

Great, so the regression tests from point 5 will be excluded.

[shawnborton]

If the year picker is no longer push to page, how will it work exactly?

[ArekChr]

From a UX perspective, I am going to implement the same animation of pushing the page, so the user will not notice any UI difference. From a DX perspective, the screen will be removed from the navigation stack.

[shawnborton]

Ah got it, that seems fine to me.

[WoLewicki]

Makes sense to me too, there will be no more tricky behaviors connected to navigation and parsing of URLs.

[mountiny]

The main difference from user perspective is that if user navigated to the year picker and they refresh, the year picker is closed as its not its own page and similarly they cannot deeplink to it. I think honestly its better since there is many edge cases we had to be considering before with the ability to deeplink etc.

[WoLewicki]

Yeah, seems like there are only upsides of this new approach if it is implemented correctly 🚀

[ArekChr]

Hi there,

Just to give you an update: I've removed the YearPickerPage from the navigation. At the moment, I'm focused on implementing the animation. I'm utilizing the Layout Animation API from Reanimated, but I've encountered an issue with the exiting animation.

The problem is that the exiting animation doesn't seem to work at all after the component unmounts. I've set the entering animation to SlideInRight and the exiting animation to SlideOutLeft. However, while the component is unmounting, I receive a warning message:

Warning: Overriding previous layout animation with new one before the first began: <RCTLayoutAnimationGroup: 0x60000148f060; creatingLayoutAnimation: (null); updatingLayoutAnimation: <RCTLayoutAnimation: 0x6000001b8900; duration: 0.250000; delay: 0.000000; property: (null); springDamping: 0.000000; initialVelocity: 0.000000; animationType: 5;>; deletingLayoutAnimation: (null)> -> (null).

@WoLewicki, do you happen to know any potential solutions for this issue? If not, I'll implement own animation logic of sliding the container from right to left when mounting and right to left when unmounting.

The current implementation is below:

{this.state.isYearPickerVisible && (
    <Portal hostName="RightModalNavigator">
        <ScreenSlideAnimation>
            <YearPickerPage
                onClose={() => this.setState({isYearPickerVisible: false})}
                min={moment(this.props.minDate).year()}
                max={moment(this.props.maxDate).year()}
                currentYear={parseInt(this.state.selectedYear, 10)}
            />
        </ScreenSlideAnimation>
    </Portal>
)}

And here is the function ScreenSlideAnimation:

function ScreenSlideAnimation({children}) {
    return (
        <Animated.View
            key="yearPicker"
            entering={SlideInRight}
            exiting={SlideOutLeft}
            style={StyleSheet.absoluteFillObject}
        >
            {children}
        </Animated.View>
    );
}

https://github.com/Expensify/App/assets/24796318/337790fe-f675-4838-923a-520cbc615e65

edit:

I've also tried a suggested potential fix from a discussion here. It involved adding flex: 1 to the contentContainerStyle in OptionsSelector, which is then passed to SelectionList. Unfortunately, this didn't resolve the issue as expected.

I've also looked into the warning regarding Overriding previous layout animation with a new one before the first began. It appears to be related to the keyboard. However, even when hiding the keyboard before unmounting the component, which does clear the warning, it, unfortunately, doesn't resolve the primary issue with the exiting animation.

[WoLewicki]

Does it only happen on the last screen? Or for the one with calendar also?

[mountiny]

I have asked SWM for help on this one

[WoLewicki]

Also, how will it work on web? IIRC, LayoutAnimations are not supported there. I can see that the App uses react-native-animatable in some of similar flows.

[ArekChr]

Does it only happen on the last screen? Or for the one with calendar also?

This happen only in the Year Picker

[melvin-bot[bot]]

@burczu @kevinksullivan @ArekChr @WoLewicki @mountiny this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[ArekChr]

Hello, I wanted to share my update. The refactoring is complete, and the only missing step is animation. I have rewritten the animation using a basic reanimated API. It works well on mobile devices, but I am encountering issues on the web where the entire window moves when the animation starts. This screen is nested near RigthModalNavigator using Portal. @WoLewicki, do you know why this is happening on the web or how to fix that?

The location where the YearPicker component is sent is within the RigthModalNavigator function, specifically inside the PortalHost component.

function RigthModalNavigator() {
    return (
        <>
            <Stack.Navigator>
                {/* other screens */}
            </Stack.Navigator>
            <PortalHost name="RigthModalNavigator" />
        </>
    );
}

Year Picker Animation

const width = Dimensions.get('window').width;

const ScreenSlideAnimation = React.forwardRef(({children, testID}, ref) => {
    const [visible, setVisible] = React.useState(false);
    const translateX = useSharedValue(width);

    React.useImperativeHandle(
        ref,
        () => ({
            close: () => {
                translateX.value = withTiming(width, {duration: 500}, (finished) => {
                    if (!finished) {
                        return;
                    }
                    runOnJS(setVisible)(false);
                });
            },
            open: () => setVisible(true),
        }),
        [],
    );

    React.useEffect(() => {
        if (!visible) {
            return;
        }
        translateX.value = withTiming(0, {duration: 500});
    }, [visible]);

    const animatedStyle = useAnimatedStyle(() => ({transform: [{translateX: translateX.value}]}), [translateX.value]);

    if (!visible) return null;

    return (
        <Portal hostName="RigthModalNavigator">
            <Animated.View
                testID={testID}
                style={[StyleSheet.absoluteFillObject, animatedStyle]}
            >
                {children}
            </Animated.View>
        </Portal>
    );
});

https://github.com/Expensify/App/assets/24796318/2f1f7540-6591-487c-a236-9d38f5be13ac

https://github.com/Expensify/App/assets/24796318/641ad55e-3274-4c44-826f-98f4b8635a44

[mountiny]

Assinging Fedi as a C+ from the PR.

Arek is ooo for two weeks so someone else from Callstack will take over

SWM was not able to reproduce the issue with the animation on the year picker

[thiagobrez]

Hey! I'm Thiago from Callstack - expert contributor group - and I will be taking this over from Arek while he is OOO

I'm somewhat familiar with the work done here, but let me read through everything and take a closer look at the PR changes before answering further 🙇🏻

[thiagobrez]

Sending updates in the PR

[thiagobrez]

More updates in the PR :)

[melvin-bot[bot]]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot[bot]]

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

• https://github.com/Expensify/App/pull/21792

If no regressions arise, payment will be issued on 2023-07-20. :confetti_ball:

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• [ ] External issue reporter
• [ ] Contributor that fixed the issue
• [ ] Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot[bot]]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [ ] [@fedirjh] The PR that introduced the bug has been identified. Link to the PR:
• [ ] [@fedirjh] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [ ] [@fedirjh] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [ ] [@fedirjh] Determine if we should create a regression test for this bug.
• [ ] [@fedirjh] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [ ] [@kevinksullivan] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[fedirjh]

@mountiny Do we need a checklist for this issue ?

[mountiny]

No

[mountiny]

@kevinksullivan this should be $1000 to @fedirjh for review and testing. thank you 🙇

[mountiny]

@kevinksullivan this is ready to be paid

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~018d352aca1b0ce802

[melvin-bot[bot]]

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

[kevinksullivan]

Just labeling to get the upwork job set up.

[melvin-bot[bot]]

Current assignees @ArekChr and @fedirjh are eligible for the External assigner, not assigning anyone new.

[kevinksullivan]

Offer sent @fedirjh ^^ . Let me know when you accept!

[fedirjh]

@kevinksullivan Thank you! Accepted

[JmillsExpensify]

Woo! Awesome work on this one. Excited to have it in the wild.

[mountiny]

@kevinksullivan All done here, can we close this?

[kevinksullivan]

All set / paid