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

Range and Multi selection modes #508

Closed
wants to merge 9 commits into from
Closed

Conversation

RedHatter
Copy link

@RedHatter RedHatter commented Jul 6, 2018

Closes #364

This pull request implements the ability to select a range of dates or multiple individual dates for the DatePicker component.

It works by changing the internal values to work on arrays instead of single date objects. This allows multiple dates (and ranges expressed as [ startDate, endDate ]) to be processed with minimal changes.

Range and Multi selection modes could be implmented for the TimePicker and DateTimePicker components in a similar manner.

New props

A few new props have been added to facilitate smooth operation.

  • Range Enables range selection mode.
  • Multi Enables multiple selection mode.
  • formatSeperator A string to using when combining multiple dates. Defaults to ', '.
  • initialFocusedDate When explicitly set to false no initial date is selected. Selecting today by default is non-optimal for range selection mode in particular.

@RedHatter RedHatter changed the base branch from develop to master July 6, 2018 22:17
@RedHatter RedHatter changed the title Range selection WIP Range and Multi selection modes Jul 13, 2018
@RedHatter RedHatter changed the base branch from master to develop July 13, 2018 19:42
@ghost
Copy link

ghost commented Jul 22, 2018

Does anyone know, if this feature is going to be available any time soon or how to implement it myself?

@dmtrKovalenko
Copy link
Member

@floanwelt I am not really sure is this feature is still in WIP or not. Anyway I will wait till conflicts will be resolved and will review this one

newDate = utils.setMinutes(newDate, utils.getMinutes(date[0]));
}

if (multi) {
Copy link

Choose a reason for hiding this comment

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

Might this work better as a dictionary? I'm not sure if this would work across the date util libraries, but something like this has worked for my implementation elsewhere:

selection = {
  ...selection,
  [day]: !selection[day],
};

Copy link
Author

Choose a reason for hiding this comment

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

You are correct that a dictionary would be more efficient for multi select mode but map and a few other functions of arrays allows us to reuse most of the code for all three modes. As far as I see the small efficiency gain of a dictionary would not be worth the added complexity.

@@ -25,6 +25,8 @@ export class DatePicker extends PureComponent {
utils: PropTypes.object.isRequired,
shouldDisableDate: PropTypes.func,
allowKeyboardControl: PropTypes.bool,
multi: PropTypes.bool,
Copy link

Choose a reason for hiding this comment

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

It seems as though these types of selections are wrappers that replace the renderDay implementation. Might it make sense to implement a separate MultiDatePickerWrapper and RangeDatePickerWrapper?

Copy link
Author

@RedHatter RedHatter Jul 23, 2018

Choose a reason for hiding this comment

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

I went that direction with my first attempt. I got it somewhat working but there were issues and ended up hacky all around. Rolling the features into the core seamed to be a better solution. Honestly it required a lot less modification than I expected.

@RedHatter
Copy link
Author

RedHatter commented Jul 23, 2018

All the features are implemented but as @dmtrKovalenko mentioned I still need to fix conflicts and tests. I'm not sure if I'll have time for a while.

@RedHatter
Copy link
Author

RedHatter commented Jul 23, 2018

Apparently I did have time to fix the conflicts.

@ghost
Copy link

ghost commented Jul 27, 2018

@RedHatter Thank you for your time and work :-) If there is sth I can help with, I am happy to help out.

@tgBryanBailes
Copy link

@dmtrKovalenko Any word on when this might be reviewed?

@RedHatter Looks like there are still some unresolved conflicts, not sure if these happened after you said they were resolved or not.

I would love to see this get merged.

@dmtrKovalenko
Copy link
Member

@RedHatter I apologize for long response, but I need to close this PR.
I have a lot of ideas how to separate the logic of range pickers to not bundle them with default pickers - that should save bundle size.

I will reuse as many code from your PR as possible. Thank you for your job. :)

@sebastianqdot
Copy link

I was trying to do something like this and was bummed out finding this PR was rejected. It doesn't look like the bundle size will change much noting it has a balance of +160 lines and no new dependencies.

To me it feels like this is a great addition to the repo and it definitely will come handy for many people, knowing range pickers are becoming more and more common these days. Chances of giving this another look?

@andres-amor
Copy link

@dmtrKovalenko is there another repo or library that holds this separate logic of range pickers? Somewhere where we can work on?. We really need that feature, it would be great to collaborate instead of just fork and apply these @RedHatter changes.

If there is no place for this on the roadmap on the short-term, I suggest to apply @RedHatter ideas, as @sebastianqdot says, 180 lines will not affect the bundle size critically.

@RedHatter
Copy link
Author

As discussed in #364 I wrote a simple wrapper that does the same thing without modifying the library. Gist

I don't know what @dmtrKovalenko's plans are, but it's a good solution until something official is released.

@cevr
Copy link

cevr commented Apr 12, 2019

@RedHatter, would you have an idea on how to make this wrapper work with the DateTimePicker?

@Germain67
Copy link

Any plans on having this in v3?

@dmtrKovalenko
Copy link
Member

@Germain67 I will work on it right after v3 released

@iccube-real
Copy link

@dmtrKovalenko , do you have an update on this ? with v4 and the week example it looks very close to be there.

Thanks for you work.

@dmtrKovalenko
Copy link
Member

I am going to work on multiselsction mode after alpha.5.

@mui mui deleted a comment from steplerbox Feb 19, 2020
@diegovillacis10
Copy link

diegovillacis10 commented Aug 5, 2020

Hello, are there any updates on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants