Feat: New Date Picker Design #15343

ArekChr · 2023-02-22T10:20:50Z

Details

This Pull Request introduces the new implementation of DatePicker. At this moment, it is implemented only in Date of Birth screen.

Form (forward ref) has been fixed for CalendarPicker ref forwarding
OptionsSelection (forward keyboardType) has been updated to change the keyboard type in the selection text input
BaseTextInput has been updated by adding a right generic icon to display the calendar icon in the DOB input

Fixed Issues

$ #14322
PROPOSAL: #14322 (comment)

Tests

Verify that no errors appear in the JS console

Enter the Date of Birth page
Click on the input
Change the month using the arrows
Click the year
After navigating to new screen select another year
It should go back to screen with calendar open and new year in the picker
Select the date

Offline tests

No need to test offline

QA Steps

Verify that no errors appear in the JS console

Enter the Date of Birth page
Click on the input
Change the month using the arrows
Click the year
After navigating to new screen select another year
It should go back to screen with calendar open and new year in the picker
Select the date

PR Author Checklist

Screenshots/Videos

Web

WEB-CHROME.mov

Mobile Web - Chrome

ANDROID-CHROME.mov

Mobile Web - Safari

IOS-SAFARI.mov

Desktop

DESKTOP.mov

iOS

IOS.mov

Android

ANDROID.mov

MelvinBot · 2023-02-22T10:20:55Z

Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers!

ArekChr · 2023-02-22T10:24:50Z

#15208

ArekChr

Overall looks good. I left some minor comments, good job 🙌

src/components/CalendarPicker/index.js

src/components/NewDatePicker/index.native.js

src/stories/Form.stories.js

MelvinBot · 2023-03-02T12:06:57Z

@Beamanator @mananjadhav One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

TMisiukiewicz · 2023-03-02T14:49:05Z

also tagging @shawnborton for design review

shawnborton · 2023-03-02T15:03:48Z

Looking good! A couple of minor items to address:

Let's use a calendar icon to the right of the date input field. You can use this svg here:
calendar.svg.zip

For the arrows, maybe we should give them a hover color? Perhaps even just using our textSupporting color would be enough.

For the year picker screen, I don't think we need to have the "Please select a year" text - I think it's pretty obvious what you need to do on that screen so that text doesn't really offer much, it just takes up space.

shawnborton · 2023-03-02T15:09:11Z

Also, on the year screen, should we auto focus the search input? cc @trjExpensify not sure if we have a rule of them there.

Also, I notice that the list options stretch full width so the border goes to the edge of the screen. I think we want to fix this so that the borders line up within the left edge of the text. cc @cristipaval - did we fix this globally yet?

This is what we ideally want:

mountiny

@TMisiukiewicz @ArekChr Love that we got tests here already

There is one thing I would like to raise which is I think not a great UX. The Month section arrows are positioned to the right from the month text and hence if you change the month the position of the arrows moves too. This means you cannot quickly skip through them as you wont be able to click on the arrows quickly. you can see it in the web video here https://user-images.githubusercontent.com/24796318/222424665-7e5caae7-0d5a-4696-afbb-df53806ff1eb.mov

should we put the arrows on the left? or something else @shawnborton @Beamanator

src/components/CalendarPicker/ArrowIcon.js

src/components/CalendarPicker/calendarPickerPropTypes.js

src/components/CalendarPicker/generateMonthMatrix.js

src/components/NewDatePicker/index.native.js

src/pages/YearPickerPage.js

src/pages/settings/Profile/PersonalDetails/DateOfBirthPage.js

shawnborton · 2023-03-02T15:13:09Z

I agree with that @mountiny and I actually think @JmillsExpensify designed an updated version of this at some point where the year is on the left side so it matches the formatting we do (YYYY-MM-DD). So let's do this:

Which I think solves that problem?

mountiny · 2023-03-02T15:13:36Z

@shawnborton yep, love that, thanks!

trjExpensify · 2023-03-02T23:42:29Z

Also, on the year screen, should we auto focus the search input? cc @trjExpensify not sure if we have a rule of them there.

Yep, agreed we should! Another instance of this particular list view page in production is the currency page in the request money flow.

s77rt

There is a lot to be improved here, first initial tests shows the date picker working well but I have few things to note for now:

Arrows keep changing positions, making it hard to fast month switch.
When a date is selected and you open the calendar, it will always point to last date, it should point to the current selected date.
Clicking on year (while field is empty) will yield a form error.
Can't clear date.
backTo param accepts any value, not that I found a bug in that, just worth mentioning maybe we should use a whitelist to be extra safe
The calendar (on web) is always rendered, it's just invisible (opacity is 0), so we can't use it with any other field? Is the date picker supposed to be rendered in it's separated page? does not seem to make sense.

src/components/CalendarPicker/calendarPickerPropTypes.js

src/components/CalendarPicker/ArrowIcon.js

src/components/CalendarPicker/generateMonthMatrix.js

src/components/CalendarPicker/index.js

src/components/NewDatePicker/index.js

src/pages/settings/Profile/PersonalDetails/DateOfBirthPage.js

src/components/NewDatePicker/index.native.js

mountiny · 2023-03-06T10:38:10Z

Can't clear date.

I think for now we decided to go without an option to clear, we can see later if the user experience will be bad, essentially the dates should not be optional I believe.

@s77rt great review 🙌

Beamanator · 2023-03-06T15:50:54Z

I'm planning to hold off reviewing till more of the outstanding comments get resolved 👍 Or if they're already addressed, can you please resolve them @ArekChr ? 🙏

ArekChr · 2023-03-06T16:09:29Z

@Beamanator Yes, I will finish probably tomorrow

mountiny

Let's ship this 🎉

comments were addressed

mountiny · 2023-03-20T16:33:54Z

I think we got approvals of Carlo, s77rt and Alex's comments were addressed so I am going to ship this :shiny:

OSBotify · 2023-03-20T16:37:46Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2023-03-21T16:06:19Z

🚀 Deployed to staging by https://github.com/mountiny in version: 1.2.88-0 🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2023-03-21T18:11:10Z

🚀 Deployed to staging by https://github.com/mountiny in version: 1.2.88-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

tienifr · 2023-03-23T11:08:54Z

@mountiny @luacmartins This issue seems to be a regression caused by this PR.

I just made a quick proposal to fix it

Proposal

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

Place holder date format is incorrect in Spainese

What is the root cause of that problem?

In this PR, we are setting the placeholder in DateOfBirthPage as a constant (YYYY-MM-DD)

App/src/components/NewDatePicker/index.js

Line 130 in e1e2759

placeholder={this.props.placeholder || CONST.DATE.MOMENT_FORMAT_STRING}

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

In Expensify/App/src/components/NewDatePicker/index.js
We should use withLocalize to wrap the component

withLocalize(React.forwardRef((props, ref) => (
    /* eslint-disable-next-line react/jsx-props-no-spreading */
    <NewDatePicker {...props} innerRef={ref} />
)));

don't forget to import new things and update the propsType

import withLocalize, {withLocalizePropTypes} from '../withLocalize';

const propTypes = {
    ...withLocalizePropTypes,
    ...datePickerPropTypes,
};

and then update the placeholder
placeholder={this.props.placeholder || this.props.translate('common.dateFormat')}

What alternative solutions did you explore? (Optional)

We also can pass the translate function from the parent component instead of using withLocalize

Result

mountiny · 2023-03-23T11:38:10Z

Nice, exported the issue, do you want to post the proposal there too

tienifr · 2023-03-23T11:43:12Z

@mountiny I just posted a proposal

OSBotify · 2023-03-24T23:59:52Z

🚀 Deployed to production by https://github.com/AndrewGable in version: 1.2.88-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

OSBotify · 2023-03-27T09:46:16Z

🚀 Deployed to production by https://github.com/AndrewGable in version: 1.2.88-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

Santhosh-Sellavel · 2023-04-06T22:34:02Z

src/pages/YearPickerPage.js

+                <OptionsSelector
+                    textInputLabel={this.props.translate('yearPickerPage.selectYear')}
+                    onChangeText={this.filterYearList}
+                    keyboardType={CONST.KEYBOARD_TYPE.NUMBER_PAD}
+                    maxLength={4}
+                    value={this.state.inputText}
+                    sections={[{data: this.state.yearOptions, indexOffset: 0}]}
+                    onSelectRow={option => this.updateSelectedYear(option.value)}
+                    initiallyFocusedOptionKey={this.currentYear.toString()}
+                    hideSectionHeaders
+                    optionHoveredStyle={styles.hoveredComponentBG}
+                    shouldHaveOptionSeparator
+                    contentContainerStyles={[styles.ph5]}
+                />


Here we added a Search option but failed to handle no results case for the search.

Which lead to this issue

roryabraham · 2023-04-10T23:21:43Z

src/components/CalendarPicker/index.js

+import getButtonState from '../../libs/getButtonState';
+import * as StyleUtils from '../../styles/StyleUtils';
+
+class CalendarPicker extends React.PureComponent {


There's a huge amount of comment history in this PR, so apologies if I missed something, but imo this CalendarPicker component should not have been separated from the NewDatePicker. Doing so actually makes things a bit more complicated.

I agree. It would have been good to get some wider input before introducing large changes like this.

It was separated to two components as initially we had a logic which was hiding and opening the date picker once user focused into the date input. So it was clearer from developer perspective that way I would say.

Down the road we have decided to remove this logic and the calendar is permanently shown now so it should be easier to keep it all in one component now

Made an issue to track the improvements here if you want to follow along

There was a bunch of follow up PRs to this component since this PR got merged so I am sure we could use some clean up and refactoring as well.

roryabraham · 2023-04-10T23:24:42Z

src/components/NewDatePicker/datePickerPropTypes.js

+} from '../TextInput/baseTextInputPropTypes';
+import CONST from '../../CONST';
+
+const propTypes = {


These prop types are not imported anywhere except for src/components/NewDatePicker/index.js. So they should not have been moved to their own file and instead should be in the same file as the component they're relevant to. This also means that we would no longer need src/components/NewDatePicker to be a directory, and could just have src/components/NewDatePicker.js.

roryabraham · 2023-04-10T23:58:22Z

src/components/NewDatePicker/index.js

+        this.showPicker = this.showPicker.bind(this);
+        this.hidePicker = this.hidePicker.bind(this);
+
+        this.opacity = new Animated.Value(0);


I'd like to normalize using Reanimated as the default animation library for the sake of both performance and simplicity. This could've been a bit simpler by just using default Reanimated LayoutAnimations:

import Animated, {FadeIn, FadeOut} from 'react-native-reanimated'; ... ... <Animated.View onMouseDown={(e) => { // To prevent focus stealing e.preventDefault(); }} style={[styles.datePickerPopover, styles.border]} entering={FadeIn} exiting={FadeOut} > <CalendarPicker /> </Animated.View>

and I think that's it!

I think thats a great suggestion

thesahindia · 2023-05-05T19:59:48Z

src/components/CalendarPicker/index.js

+        super(props);
+
+        this.monthNames = moment.localeData(props.preferredLocale).months();
+        this.daysOfWeek = moment.localeData(props.preferredLocale).weekdays();


This caused a regression.

Text wasn't changing when switched to a different locale through another device/tab

situchan · 2023-11-02T14:40:52Z

src/components/CalendarPicker/index.js

+        const hasAvailableDatesNextMonth = moment(this.props.maxDate).endOf('month').startOf('day') > moment(this.state.currentDateView).add(1, 'M');
+        const hasAvailableDatesPrevMonth = moment(this.props.minDate).startOf('day') < moment(this.state.currentDateView).subtract(1, 'M').endOf('month');


This comparison was not accurate. More details about the root cause: #28622 (comment)

This was referenced Feb 28, 2023

Feat: New Date Picker Design #15208

Closed

[HOLD for payment 2023-03-31] Implement New Date Picker Design #14322

Closed

mountiny assigned ArekChr Feb 28, 2023

TMisiukiewicz force-pushed the feat/new-date-picker-design branch 2 times, most recently from 4442a98 to 6c1d51f Compare March 1, 2023 11:58

ArekChr commented Mar 1, 2023

View reviewed changes

src/components/CalendarPicker/index.js Outdated Show resolved Hide resolved

src/components/NewDatePicker/index.native.js Outdated Show resolved Hide resolved

src/stories/Form.stories.js Outdated Show resolved Hide resolved

ArekChr marked this pull request as ready for review March 2, 2023 12:06

ArekChr requested a review from a team as a code owner March 2, 2023 12:06

melvin-bot bot requested review from Beamanator and mananjadhav and removed request for a team March 2, 2023 12:06

mountiny requested review from mountiny and s77rt March 2, 2023 14:34

shawnborton self-requested a review March 2, 2023 14:58

mountiny reviewed Mar 2, 2023

View reviewed changes

TMisiukiewicz force-pushed the feat/new-date-picker-design branch from f1c85b7 to 03c62d2 Compare March 3, 2023 11:20

s77rt suggested changes Mar 3, 2023

View reviewed changes

mountiny requested a review from s77rt March 7, 2023 16:19

mountiny approved these changes Mar 20, 2023

View reviewed changes

mountiny merged commit bb8dff1 into Expensify:main Mar 20, 2023

OSBotify mentioned this pull request Mar 21, 2023

Deploy Checklist: New Expensify 2023-03-21 #16345

Closed

28 tasks

koko57 mentioned this pull request Mar 21, 2023

Refactor the way of displaying errors in several forms #15978

Merged

55 tasks

This was referenced Mar 21, 2023

Personal Details - Selected month changes to current month after selecting a year in the picker #16358

Closed

Android - Profile-Previously selected data is highlighted with circle, but newly selected-with square #16375

Closed

s77rt mentioned this pull request Mar 22, 2023

[HOLD 14322] [$1000] Selecting a new date on the date picker and then clicking on CANCEL doesn’t show the green border #15080

Closed

1 task

kavimuru mentioned this pull request Mar 29, 2023

Arrow next to year is not working properly #16712

Closed

6 tasks

eVoloshchak mentioned this pull request Apr 3, 2023

[HOLD] [$4000] DatePicker doesn't have green border while choosing the date. #14140

Closed

1 task

Santhosh-Sellavel reviewed Apr 6, 2023

View reviewed changes

Santhosh-Sellavel mentioned this pull request Apr 6, 2023

[HOLD for payment 2023-04-13] [$1000] An error message is not displayed when an incorrect number format is used while selecting the year. #16713

Closed

6 tasks

akinwale mentioned this pull request Apr 7, 2023

[$1000] Inconsistent placeholder in incorporation date in mweb chrome vs android #16983

Closed

6 tasks

roryabraham reviewed Apr 11, 2023

View reviewed changes

thesahindia reviewed May 5, 2023

View reviewed changes

thesahindia mentioned this pull request May 5, 2023

[PAID] [$1000] The months are not dynamically updated to Spanish language when language preference is changed from the other device #17098

Closed

6 tasks

ArekChr mentioned this pull request Jun 19, 2023

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

Closed

mananjadhav mentioned this pull request Jul 19, 2023

[HOLD for payment 2023-07-20] [$1000] Dev: Web - Workspace - Calendar disappears when clicking on it #20973

Closed

6 tasks

situchan reviewed Nov 2, 2023

View reviewed changes

situchan mentioned this pull request Nov 2, 2023

[HOLD for payment 2023-10-23] [$500] Request money - On last day of month, going back to previous month disables forward button in request money date field #28622

Closed

6 tasks

fedirjh mentioned this pull request Feb 6, 2024

[HOLD for payment 2024-03-06] [$500] Date of birth - Lower portion of year in the list only appears after some delay #34763

Closed

6 tasks

		const hasAvailableDatesNextMonth = moment(this.props.maxDate).endOf('month').startOf('day') > moment(this.state.currentDateView).add(1, 'M');
		const hasAvailableDatesPrevMonth = moment(this.props.minDate).startOf('day') < moment(this.state.currentDateView).subtract(1, 'M').endOf('month');

Feat: New Date Picker Design #15343

Feat: New Date Picker Design #15343

Conversation

ArekChr commented Feb 22, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

MelvinBot commented Feb 22, 2023

ArekChr commented Feb 22, 2023

ArekChr left a comment

Choose a reason for hiding this comment

MelvinBot commented Mar 2, 2023

TMisiukiewicz commented Mar 2, 2023

shawnborton commented Mar 2, 2023 • edited Loading

shawnborton commented Mar 2, 2023

mountiny left a comment

Choose a reason for hiding this comment

shawnborton commented Mar 2, 2023

mountiny commented Mar 2, 2023

trjExpensify commented Mar 2, 2023

s77rt left a comment • edited Loading

Choose a reason for hiding this comment

mountiny commented Mar 6, 2023

Beamanator commented Mar 6, 2023

ArekChr commented Mar 6, 2023

mountiny left a comment

Choose a reason for hiding this comment

mountiny commented Mar 20, 2023

OSBotify commented Mar 20, 2023

OSBotify commented Mar 21, 2023

OSBotify commented Mar 21, 2023

tienifr commented Mar 23, 2023

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Result

mountiny commented Mar 23, 2023

tienifr commented Mar 23, 2023

OSBotify commented Mar 24, 2023

OSBotify commented Mar 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mountiny Jun 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArekChr commented Feb 22, 2023 •

edited

Loading

shawnborton commented Mar 2, 2023 •

edited

Loading

s77rt left a comment •

edited

Loading

mountiny Jun 12, 2023 •

edited

Loading