UX-408 Date picker with shortcut panel

[allison-c]

🛠 Purpose

Adding shortcut panel to date picker component.

✏️ Notes to Reviewer

• Added shortcut panel for calendar
• The calendar is bigger
• The initial displayed date will be in spelled out format
• Added prop humanFormat for customize spelled out format
• Added a new component DatePicker.Popover
• Added knobs in storybook with showcase story
• Fixed popover bug in Edge

🖥 Screenshots

GIF

Popover bug in Edge:

It was because getBoundingClientRect won't return x y in Edge and IE11

[allison-c]

@nahumzs could you review this part? Found a bug in Microsoft Edge and fixed it.

[alexzherdev]

Not sure what this means.

[allison-c]

We want to rerender react-date calendar when the user is typing in the input. This prop is the parsing result(in moment object) from user's input, and I use it to rerender calendar

[mikrotron]

What makes it possible though? I probably don't mind the name, but this comment is the opportunity to explain that name choice.

[allison-c]

maybe predictedDate ? or prediction ?

[alexzherdev]

What does this 5 mean?

[allison-c]

The selected year's index in shortcut panel

[mikrotron]

Incidental again... but would inherit work here instead, so this component remains agnostic of the app's typeface?

[allison-c]

I moved font-family: ${tokens.fontFamily.default}; to DatePicker.styles.js. I think it's good to set the default font-family otherwise I need to set it in the date picker example in storybook.

[mikrotron]

Does the component need to be a specific font? Ideally when we switch our apps to use IBM Plex, this component shouldn't have to be changed. In fact we could use it in different apps with different UI fonts, and the component should adapt. Ideally.

[allison-c]

🤔Sure! The only thing is some font doesn't works well with bold on hover, for example Helvetica Neue. It will shift a little bit as you said before. IBM Plex and Lato doesn't have this issue. I will just use inherit then. I can add the default font in storybook if that looks bad.

[nahumzs]

I just noticed that moment is a dependency, but shouldn't be a peer-dependency? cc @alexzherdev

[nahumzs]

I think the linter should be warning you that you need to add keepFocus as part of the arrayDependency isn't?

[nahumzs]

this could be a hook, but that scope would be totally out of this PR. seem ok to me.

[alexzherdev]

I just noticed that moment is a dependency, but shouldn't be a peer-dependency? cc @alexzherdev

Hmm did some research. TL;DR yeah let's make it a peerDependency. Unless we're really using some super new features, let's depend on a broad range like ^2.0.0 to avoid "unmet peer dependency" warnings in the apps.

---

I had thought that in this case peerDep would only help ensure one copy of moment was in the tree for bundle size purposes (and not break anything, unlike with React, where two copies could break the app). But turns out that two copies of moment can also cause breakage when you do checks on whether something is a moment object:

import moment from 'moment';
moment.isMoment(momentObj); // or momentObj instanceof moment

If momentObj is created by a different copy of moment than moment, this check will fail.

[allison-c]

I just noticed that moment is a dependency, but shouldn't be a peer-dependency? cc @alexzherdev

Hmm did some research. TL;DR yeah let's make it a peerDependency. Unless we're really using some super new features, let's depend on a broad range like ^2.0.0 to avoid "unmet peer dependency" warnings in the apps.

I had thought that in this case peerDep would only help ensure one copy of moment was in the tree for bundle size purposes (and not break anything, unlike with React, where two copies could break the app). But turns out that two copies of moment can also cause breakage when you do checks on whether something is a moment object:

import moment from 'moment';
moment.isMoment(momentObj); // or momentObj instanceof moment

If momentObj is created by a different copy of moment than moment, this check will fail.

Thanks Alex! Nahum and I also had a small conversion in person. I was asking why not put @paprika/raw-button in peerDep as well. Then we end up with we don't want to force consumers to upgrade peerDep so frequently, so paprika components should be dependency but @paprika/icon might be valuable to be a peerDep because we won't change it much once it's stable

[alexzherdev]

I was asking why not put @paprika/raw-button in peerDep as well. Then we end up with we don't want to force consumers to upgrade peerDep so frequently, so paprika components should be dependency but @paprika/icon might be valuable to be a peerDep because we won't change it much once it's stable

I'd say that's overdoing it. Part of the reason we switched to a monorepo was the ability to have multiple versions of the same component in the tree if we need to—peerDep undoes that.
I don't think the frequency of updates to a package is an indicator of a peerDep. The indicator is usually "I need one and only one copy of this package in the tree otherwise my code breaks".
Additionally, having peerDeps is a hassle because increasing a peerDep's version is a breaking change for your package.

[nahumzs]

This seems good enough for the first version seems there is a little concern with the prop possibleDate. Though nobody reply or comment after 13 days, so I will take it as the prop name has a low "giveAFu" value on the thermometer of the reviewers.