[DatePicker] Hookify

[amrocha]

WHY are these changes introduced?

We're updating our components to use hooks and functions instead of classes and HoCs like before, this is part of that work

#1995

WHAT is this pull request doing?

Hookifies Day and DatePicker

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:

import React from 'react';
import {Page} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      {/* Add the code you want to test in here */}
    </Page>
  );
}

🎩 checklist

• Tested on mobile
• Tested on multiple browsers
• Tested for accessibility
• Updated the component's README.md with documentation changes
• Tophatted documentation changes in the style guide
• For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

[AndrewMusgrave]

Looks good! Left a few small comments

[AndrewMusgrave]

Are we sure we can remove this logic?

[amrocha]

Yeah, I'm not even sure why we're doing this.

This is just doing an actual value comparison, instead of just trusting that if the reference is different then something has changed. I think whoever wrote this was forced to do it like this to avoid re-rendering unnecessarily, but now I can just replace this with the useEffect hook in line 69 that only fires if the prop actually changed.

The only case I can think that this doesn't account for is someone selecting the same date twice in a row, and triggering a re-render of the component. But I tried that and it seemed to work fine, so

[AndrewMusgrave]

Looking back the code seems to fix this issue and a quick 🎩 shows it's still 💯 ✅

useEffect hook in line 69 that only fires if the prop actually changed

Funny enough we could have done this in a class based component as well.

  useEffect(() => {
    setFocusDate(undefined);
  }, [selected]);

Is essentially the same as

componentDidUpdate(prevProps) {
 if (!Object.is(prevProps.selected, this.props.selected) 
    this.setState({focusedDate: undefined})
}

[AndrewMusgrave]

It looks like onChange was previously called when the setState for hover/focus had resolved. Any idea why, and do you think it'll change anything?

[amrocha]

Not sure, could just be trying to prevent race conditions. I expect this to be basically equivalent.

Do you think it'd be better to call onChange in a different useEffect function?

[AndrewMusgrave]

I think we'll be ok, I was just wondering if anything stood out while you were working on this.

[BPScott]

All looks good, one query inline that might be a dealbreaker though

[BPScott]

Does this mean that onHover/onFocus events that consumers may pass in now need to deal with the day possibly being undefined? That type widening is potentially a breaking change as they would never have had to handle undefined being passed in. Things may break if we give them undefined but their function is always expecting a Date.

Is there some way we can keep these two arguments as required?

[amrocha]

Day is not a public member of the Polaris API, so the only consumers are internal, and this doesn't throw any type errors so that's not a concern.

I think we're safe to make any changes we want to private parts of our API. Do you disagree?

[BPScott]

Good point on this not actually being public, so the breaking change thing is moot.

It looks like we can keep onFocus?(day: Date) without any extra changes needed so that'd be easy to do.

I was thinking we could remove the onHover from the empty day but it looks like that has knock of effects for how selecting a range (with <DatePicker allowRange>) works so it looks like we need to keep that.

[amrocha]

I'm also wary of making too many changes in this PR, I'm already making a few changes to the flow here and there and I'm not super familiar with the Datepicker so I'm trying to keep everything roughly the same

[BPScott]

Change the onFocus?(day: Date) to onFocus(day: Date) as it looks like that doesn't cause any type errors, rebase on master and reformat (we've had a prettier version bump since this was opened) and this is good to go