Refactor date picker to be compatible with form by sig5 · Pull Request #7627 · Expensify/App

sig5 · 2022-02-08T06:26:01Z

Details

An optional isFormInput prop.

If isFormInput is true, require a inputID prop. Use [getInputIDPropTypes] (https://github.com/Expensify/App/blob/1860b590114991d1819a5590608d26c32337de56/src/libs/FormUtils.js#L7)[] to enforce this propType rule.

Add an optional shouldSaveDraft prop. Defaults to false.

Importing the baseTextInputPropTypes does the deal. I have added a propType for defaultValue to support both dates and strings as consistency in the value and defaultValue parameter is expected.

Make the value prop optional.

The value prop is not a required prop.

5.In the input’s onBlur method, call props.onBlur().
6.In the input’s onChange method (or equivalent e.g. onTextChange, etc), call props.onChange().
7. Remove the hasError prop.

Done!

Add an optional errorText prop. Defaults to an empty string.

Importing the baseTextInputPropTypes does the deal.

9.Update the error message to display if errorText is truth.
Error text is displayed only when non-empty!

Update the inline error display style so it is left aligned with the label and input value.

Done! Strangely it hadn't been implemented for TextInput in the PR.

Make sure that props.ref is attached to the appropriate DOM node. This could involve forwarding the ref to a child component. This is important so we can call ref.focus().

Done! Ref has been passed to TextInput elements because DatePicker doesn't natively support focus. These are handled via different methods. this.textInput.clear() and similar methods are accessible

Add the input to the test Form stories and make sure that it works. You can check this by running npm run storybook and testing the component in the example form.

Done!

13 Remove any unused code.

Done!

There are a few more discretionary changes done. I will mention those in the comments.

Newly discovered Issues:
1.The datepicker doesn't get focussed on clicking in android/iOS.
2. The storybook sidebar UI styling is broken for mWeb.

Fixed Issues

$ #7536

Tests

Run npm run storybook
Play around with the Form stories and make sure that it works as intended. Specifically, note the following:

We can enter values into the inputs.
Clicking submit displays a loading indicator in the submit button and an alert with the correct input values.
Emptying the input value and blurring it shows an error.
Clicking the fix the errors link above the submit button focus on the first input with an error.
Entering a value in the input clears the error.
Verify that server errors are displayed correctly.
Any other fishy behavior.

Run the app and test that DatePicker still works fine.

Verify that no errors appear in the JS console

The errors observed were common to TextInput. Resolution of those should rectify the ones that appear here.

QA Steps

Verify that no errors appear in the JS console

Tested On

Screenshots

Web

Kazam_screencast_00007.mp4

Mobile Web

WhatsApp.Video.2022-02-08.at.11.49.17.AM.mp4

Desktop

Kazam_screencast_00004.mp4

iOS

Kazam_screencast_00003.mp4

Android

Kazam_screencast_00002.mp4

rushatgabhane · 2022-02-08T07:38:43Z

@sig5 videos for everything except mWeb aren't working, can you please reupload? I can only see a black screen.

Edit: this is happening only on mobile devices.

rushatgabhane · 2022-02-08T12:05:44Z

The datepicker doesn't get focussed on clicking in android/iOS.

I believe this is the expected behaviour.

sig5 · 2022-02-08T12:39:09Z

Sorry for the misunderstanding, I meant that the textInput enclosed by datePicker should be focused on when we are selecting dates. See the PR Android video for context. The focus should be on DatePicker's textInput and the Keyboard should be hidden.

luacmartins · 2022-03-21T16:01:10Z

@sig5 any updates?

sig5 · 2022-03-22T17:19:34Z

Hi @luacmartins, I was waiting for clarification on this thread : #7627 (comment) . We should be good to go once we reach a conclusion here :D

luacmartins · 2022-03-22T17:38:27Z

@sig5 Sorry I missed that! Replied to it just now. Thanks for the reminder!

luacmartins · 2022-03-28T17:40:02Z

@sig5 I addressed the pending comments. Let me know if there's anything else that I missed. Thanks!

sig5 · 2022-03-28T17:43:47Z

Thank you for addressing the comments @luacmartins :D. I think the PR should be ready for review now.
cc: @rushatgabhane

rushatgabhane

@sig5 please merge main into your branch, and adress the requested changes.
Thanks!

luacmartins · 2022-03-31T15:29:43Z

@sig5 when do you think you'll be able to address @rushatgabhane's comments?

sig5 · 2022-04-01T03:50:23Z

Hi @luacmartins, sorry I missed the notifications. I think I have addressed the comments mentioned by @rushatgabhane .

luacmartins

Thanks for addressing @rushatgabhane's comments!

I'm not convinced that we need some of these additions, i.e. didComponentUpdate, value and selectedValue so I left a few more comments to try to understand this solution better.

luacmartins

Thanks for addressing my concerns @sig5!

I left a comment about renaming onChange handler.

While testing DatePicker outside the form, I noticed a strange behavior where the date gets wiped when typing it a second time (see video below). We should make sure that we are not breaking any current usages of it.

date.mov

luacmartins · 2022-04-04T19:51:26Z

+    setDate(event, selectedDate) {
+        this.setState({isPickerVisible: false});
        if (event.type === 'set') {
            this.props.onChange(selectedDate);


we renamed onChange to onInputChange passed down by the form, so we should update it accordingly within DatePicker and in all usages of DatePicker

Sure thing! I will update it.

rushatgabhane · 2022-04-07T20:03:00Z

@sig5 bump to address @ luacmartins' comments

sig5 · 2022-04-10T20:58:02Z

@luacmartins , I can't reproduce it.

rushatgabhane · 2022-04-11T18:07:39Z

@sig5 there is definitely something strange going on.

Are you sure you can't repro this? Please let me know the details if so. (platform, steps, screenshot?)

datepicker.mov

sig5 · 2022-04-14T05:54:53Z

@rushatgabhane could you please add the steps of reproduction?

luacmartins · 2022-04-14T16:20:05Z

You can probably follow the videos, but something like:

Enter a date in the date input.
Try to edit the date
When updating the date, parts of it get wiped and have a strange behavior.

luacmartins · 2022-04-19T15:13:57Z

@sig5 any updates?

luacmartins · 2022-04-21T16:04:42Z

Hey @sig5, are there any updates here?

sig5 · 2022-04-21T17:40:52Z

Hey folks, I have been encountering some health issues for a while due to which I am unable to allocateenough time for the issues, even after trying my best. The work in my PR's is approaching completion and interested individuals can fork my branch and continue working on it. Thanks.

luacmartins · 2022-04-22T15:49:11Z

Thanks for the update @sig5 and I'm sorry to hear that. I'll pick up the issue to push this project forward. Thank you so much!

luacmartins · 2022-04-26T19:46:32Z

Closing this PR in favor of #8770

sig5 added 18 commits February 4, 2022 09:03

modified datepicker props

c3e298b

remove hasError usages

6cf0b80

added ref to ios and android

f4ef1eb

pivoted values to defaultValue for Form Compatiblity

37c8733

Passed onBlur down to TextInput.

6020b72

Passed onBlur down to TextInput.

0bcf38e

adjusted styling of error messages

e20c7da

Added prop to handle inline errors

82f8c87

Added default value of error text

906d7f4

Added value handling in desktop

345f711

Added value handling in desktop

78d1897

passed down new props

9b6f0f9

fixed desktop bugs

19f4a6b

fixed desktop bugs

8d3d32c

removed duplicate proptypes

0021a62

fixed issues

77c9d1e

conditional styling

d9b4a82

Fixed web bug

bd46454

sig5 commented Feb 8, 2022

View reviewed changes

Comment thread src/components/DatePicker/index.android.js Outdated

sig5 commented Feb 8, 2022

View reviewed changes

Comment thread src/components/DatePicker/index.js Outdated

sig5 marked this pull request as ready for review February 8, 2022 06:33

sig5 requested a review from a team as a code owner February 8, 2022 06:33

botify requested review from Luke9389 and rushatgabhane February 8, 2022 06:33

MelvinBot removed the request for review from a team February 8, 2022 06:33

sig5 added 2 commits February 8, 2022 12:19

remove old comment

9a7e686

renamed additionalstyle

beaf471

sig5 added 2 commits March 16, 2022 00:58

fix lint errors

1fa8dcc

fixed bind

87ee30a

rushatgabhane suggested changes Mar 29, 2022

View reviewed changes

sig5 and others added 2 commits March 29, 2022 15:18

Merge branch 'Expensify:main' into refactorDatePicker

a386321

addressed comments

f266233

sig5 added 2 commits April 1, 2022 08:43

fixed android state

47d7542

fixed linting

f38e10d

Merge remote-tracking branch 'origin/main' into refactorDatePicker

92a58da

luacmartins reviewed Apr 1, 2022

View reviewed changes

luacmartins requested changes Apr 4, 2022

View reviewed changes

This was referenced Apr 22, 2022

Refactor DatePicker to be compatible with Form #7536

Closed

Refactor Datepicker to work with Form.js #8770

Merged

luacmartins closed this Apr 26, 2022

Conversation

sig5 commented Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Uh oh!

Uh oh!

Uh oh!

rushatgabhane commented Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rushatgabhane commented Feb 8, 2022

Uh oh!

sig5 commented Feb 8, 2022

Uh oh!

luacmartins commented Mar 21, 2022

Uh oh!

sig5 commented Mar 22, 2022

Uh oh!

luacmartins commented Mar 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luacmartins commented Mar 28, 2022

Uh oh!

sig5 commented Mar 28, 2022

Uh oh!

rushatgabhane left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

luacmartins commented Mar 31, 2022

Uh oh!

sig5 commented Apr 1, 2022

Uh oh!

luacmartins left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

luacmartins left a comment

Choose a reason for hiding this comment

Uh oh!

luacmartins Apr 4, 2022

Choose a reason for hiding this comment

Uh oh!

sig5 Apr 9, 2022

Choose a reason for hiding this comment

Uh oh!

rushatgabhane commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sig5 commented Apr 10, 2022

Uh oh!

rushatgabhane commented Apr 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sig5 commented Apr 14, 2022

Uh oh!

luacmartins commented Apr 14, 2022

Uh oh!

sig5 commented Feb 8, 2022 •

edited

Loading

rushatgabhane commented Feb 8, 2022 •

edited

Loading

luacmartins commented Mar 22, 2022 •

edited

Loading

rushatgabhane left a comment •

edited

Loading

rushatgabhane commented Apr 7, 2022 •

edited

Loading

rushatgabhane commented Apr 11, 2022 •

edited

Loading