Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DateRangePicker] Introduce new component 🎉 #1602

Merged
merged 75 commits into from
Apr 9, 2020

Conversation

dmtrKovalenko
Copy link
Member

@dmtrKovalenko dmtrKovalenko commented Mar 24, 2020

This PR closes #364

Closes #1306
Closes #1603
Closes #1420 (but needs more documentation)

Description

Introduce new DateRangePicker component. Link to the documentation of deploy preview (https://material-ui-pickers-git-feature-daterangepicker.mui-org.now.sh/demo/daterangepicker)

DateRangePickerPreview

@vercel
Copy link

vercel bot commented Mar 24, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/mui-org/material-ui-pickers/9s0pua9w5
✅ Preview: https://material-ui-pickers-git-feature-daterangepicker.mui-org.now.sh

@cypress
Copy link

cypress bot commented Mar 24, 2020



Test summary

69 0 1 0


Run details

Project material-ui-pickers
Status Passed
Commit 12d5064
Started Apr 9, 2020 10:28 AM
Ended Apr 9, 2020 10:29 AM
Duration 01:14 💡
OS Linux Debian - 9.11
Browser Chrome 78

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One step closer to this major component. Two years in the making 06498fc :D.

Here is an early feedback around the ClickAwayListener concern you have raised. I have went a bit offroad, sorry 🙃:

  • It might be better to work on a fork, I believe the creation of branches can confuse developers that look for older versions. Basically, master, next and v3-x only as branches would reduce noise.
  • We might want to trap the focus when it lands inside the Popper, <TrapFocus disableEnforceFocus disableAutoFocus />.
  • dataset I didn't know this property existed, I have learned something. Unfortunately, applying muiPickersInput to the input isn't enough. If you click on the edge of the input, you can witness a quick open (focus) and close (click away) behavior.
  • I would suggest replacing the usage of ClickAwayListener with a blur based approach. Basically, if the focus leaves the widget, we can close it. This way, we get keyboard + click handled at once. Two birds with one stone. I have used this approach with the Autocomplete.
  • For the API, what do you think of an Autocomplete like approach?
    <DateRangePicker
        renderInput={(firstInput, secondInput) => (
            <>
                <TextField {...firstInput} />}
                {' to '}
                <TextField {...secondInput} />}
            </>
        )}
    />

It would empower a simpler composition.

  • Switching months feels a bit slow, I measure 300ms on my high-end laptop. This resonates with a past discussion we had regarding prioritizing an investigation in the runtime performance. It sounds to me like a confirmation that we will need to look into it (later) :).

@dmtrKovalenko
Copy link
Member Author

dmtrKovalenko commented Mar 25, 2020

Hahaha september of 2018 XD

Regarding to click away - data attribute is temporary solution, now working on different structure

<TrapFocus>
  <ClickAwayListener>  
    <div>
      <Input1 />
      <Input2 /> 

      <Popper disablePortal> 
           <Calendar />
      </Popper>
    </div> 
  <ClickAwayListener> 
<TrapFocus> 

This should fix all the issues

@vercel vercel bot temporarily deployed to Preview April 6, 2020 10:49 Inactive
Comment on lines 115 to 117
<div id={id} className={clsx(classes.rangeInputsContainer, className)} ref={containerRef}>
<KeyboardDateInput
{...other}
Copy link
Member

@oliviertassinari oliviertassinari Apr 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

    <div id={id} className={clsx(classes.rangeInputsContainer, className)} ref={containerRef} {...other}>
      <KeyboardDateInput

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have made a couple of suggestions. I couldn't deep dive into the code, they are too many changes. Also, the review experience on GitHub is horrible. I'm happy to provide more details on my comment where needed.

import { Dayjs } from 'dayjs';
import { Moment } from 'moment';
import { DateTime } from 'luxon';
import { makeJSDateObject } from '../../../utils/helpers';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It won't work in people's codebases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, but only one thing we can do without dramatically changes in examples - add warning comment

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it won't be the only issue. Doesn't DateRangePicker has a required context that we should problem?

I think that we will need to have a dedicated story for this. It's important all the demos can be copied and paste without any exceptions. It hasn't always been the case in the core repository, it has been painful for developers.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, this is require to interop with different libraries.
Ideally we need separate demo for each lib (maybe somehow generated as we are doing for formats)

}
);

function RangePickerWithStateAndWrapper({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it forward ref here?

lib/src/DateRangePicker/DateRangePickerInput.tsx Outdated Show resolved Hide resolved
lib/src/DateRangePicker/DateRangePickerViewDesktop.tsx Outdated Show resolved Hide resolved
lib/src/DateRangePicker/DateRangePickerViewDesktop.tsx Outdated Show resolved Hide resolved
lib/src/__tests__/setup.js Show resolved Hide resolved
lib/src/_shared/icons/ArrowDropDownIcon.tsx Outdated Show resolved Hide resolved
lib/src/views/Calendar/CalendarView.tsx Outdated Show resolved Hide resolved
@oliviertassinari oliviertassinari changed the title 🎉 DateRangePicker [DateRangePicker] Introduce new component 🎉 Apr 6, 2020
@oliviertassinari oliviertassinari added the enhancement New feature or request label Apr 6, 2020
Co-Authored-By: Olivier Tassinari <olivier.tassinari@gmail.com>
…le.tsx

Co-Authored-By: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-Authored-By: Olivier Tassinari <olivier.tassinari@gmail.com>
className={clsx(classes.rangeInputsContainer, className)}
ref={mergeRefs([containerRef, forwardedRef])}
>
<KeyboardDateInput
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion]: You might want to provide an option to disabled auto fill from browser.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have more details on the potential problem and solution? Thanks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in the root input field component, just need to disable autoComplete :P

 <InputField autoComplete="off" ... />

:)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can pass a prop inputProps={{ autoComplete: "off" }} right to the component. But this API will be changed to renderInput={props => <TextField {...props} />}

Copy link
Member

@oliviertassinari oliviertassinari Apr 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What should be the default behavior? Displaying the browser's suggestion on top of the popup could be an issue but we won't display the popup by default as in https://www.w3.org/TR/wai-aria-practices/examples/dialog-modal/datepicker-dialog.html. I was proposing two optional props openOnFocus and openOnClick to have the popup open by default. In such case, should we change the default autocomplete? I'm not sure. We do change is with the Autocomplete component, and had no people complaining, but still?

I'm in favor of keeping the current tradeoff.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason behind suggesting it being off is one scenario we encountered before:
When we have both suggested auto fill & calendar deployed(opened), it was not a very good user experience to keep both.
As mentioned, it is an optional change. I'm happy to see how the current API goes and maybe the autoOn is a more favoring solution :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
3 participants