feat(date range): added start and end date

[Grotlo]

EDIT: Fixes #69

This is for now just a suggestion. I'm not experienced enough to know if this is implemented in an efficient way (I doubt that the "clickCounter" is an actual good way of doing the function I tried to do), so if there's any ideas on how to improve upon this, I'd gladly take the feedback!

My current implementation now has a start and end-date, which means you can select two dates from the calendar if you have dateRange = true applied. selectedStart is the first date and selectedEnd is the second date. Atm I have formattedCombined which displays both dates seperated by a hyphen.

Picking the dates	After
EDIT:

^ This was done with my own test file.

[antony]

This is nice! I can't give you code tips right now due to my schedule, but looking at the UX above I think it would need some work as it's not quite obvious that a range has been selected. I think if there was a way to join all the middle days with an outline / faint background or similar that would vastly improve the usability.

But great work taking this on! I'll try to review code as soon as I can, or until @6eDesign beats me to it :)

[Grotlo]

..., but looking at the UX above I think it would need some work as it's not quite obvious that a range has been selected. I think if there was a way to join all the middle days with an outline / faint background or similar that would vastly improve the usability.

I gave it an attempt!

I think the biggest issue with my PR is how messy I've made the registerSelection function. Right now I don't have a good idea how to make it better.

Also, I don't understand what's going on with the build?

[6eDesign]

Hey @Grotlo - thanks for submitting this PR. I'll try to take a closer look when I have some more time and provide some pointers and/or collaborate with you on this UX feature.

As for the build, you're likely seeing the build fail due to linting issues. If you run npm test or npm run lint, you'll get an informative list of the linting violations. If you run npm run lint -- --fix, eslint will attempt to auto-fix as many of the linting issues as possible. This is a little annoying, I'm sure, but it helps keep a consistent format/style across multiple contributors.

[Grotlo]

As for the build, you're likely seeing the build fail due to linting issues.

Ahh, okey! I've run npm run lint -- --fix. I think it's okey now?

Also, I couldn't figure out how to edit the test file, so I made my own: https://gitlab.com/snippets/1923781

[antony]

Hey Grotlo. That's the sort of thing yeah. I think if we remove the rounding on all of the intermediate days we get something which looks a bit more like a range selection.

I'll have a go at changing the UI and maybe doing some code cleanup when I have some spare time.

[tagptroll1]

To give me opinion on it there's a few style changes I'd like, and for the click counter problem; could you not use a bool to indicate if it's the first or last click and alternate it on specific clicks?

[Grotlo]

To give me opinion on it there's a few style changes I'd like, and for the click counter problem; could you not use a bool to indicate if it's the first or last click and alternate it on specific clicks?

Fixed! Thanks! :))

I'll have another go at styling the inbetween dates, but feel free to change it fully if any of you disagree! I've made the PR ready for review now. I'm not sure what to do with the linting merge conflicts, so I'll let them be until you guys tell me what to do with them.

EDIT: I didn't have time to finish the styling, but I was close. Going home for the holidays so won't be able to finish it until next year. I'll do it if you guys can wait that long. You decide.

[antony]

@Grotlo apologies for not having time to look at this yet, but I'll definitely have a look as soon as I'm able. Time sounds like a great addition too, albeit in a different PR.

[6eDesign]

In my opinion, it would be best to have two calendars visible on the screen for date range selection. This seems to be the "standard" way of doing this if you look at other implementations and avoids a lot of usability concerns. I think this would end up being a separate component, such as DateRangePicker (it can live in the same repo but doing this as a separate component will aid in tree shaking).

I have been working on concepts for year and time selection, myself. Hoping to find the time to put together an RFC in the near future.

[antony]

@6eDesign how does two calendars work on mobile? I've seen some implementations but it's been disappointing. Do you know a good one?

[6eDesign]

Generally I have seen them stacked like so: https://www.google.com/flights?hl=en#flt=/m/02cl1..2020-01-21*./m/02cl1.2020-01-25;c:USD;e:1;sd:1;t:e

[antony]

That google one works wonderfully well. I also like the fact that if you omit end date it adds (admittedly arbitrarily) 4 days and uses that.

[Grotlo]

Hey @Grotlo - do you believe there are any blocking issues remaining with this now?

I think there is likely to be some tech-debt as pointed out by @6eDesign but I'm not too concerned about that as it can be fixed incrementally once this is merged, if he is happy with that.

There's one thing that breaks with the current implementation, and that is if you choose the last month on the month-selector. I removed the previous solution for this since it created some issues.

I've made some progress on the independent calendars, which would fix this issue anyway, but I'm currently on another task. With this in mind, what would the next step be?

Also, I imagine that the App.svelte file isn't supposed to have the daterangepicker like that.

[Grotlo]

I think it's time to revive this PR!

I've managed to make two independant calendars, that also displays vertically on mobile. The only issue there, is that both calendar navbars is on the top instead of the second one being above the second calendar. Not sure how to fix that. Also, the second month-selector doesn't quite work as intended (picking a date from either of the month-selectors sets both calendars on the same month), but nothing breaks now so. 😇 Is this good enough? Can someone help me with the month-selector issue or can we merge this?

EDIT: I had to remove the fade transition when flipping through months, since it somehow made it behave weird.

[antony]

@Grotlo With regards to the month selector selecting both, I think this is due to it being a shared store.

I've got an open issue on this calendar about moving it to the stores API rather than all of the binding it currently does. I think I will take a branch from this PR and do it there, since it's a good test case for the new functionality.

Leveraging stores and the context API means that we can have multiple totally independent calendars that don't conflict, which should fix your issue.

I'll let you know how I get on.

[antony]

Sorry, taken me a bit longer than I had hoped, of course.

I've got something mostly working with stores, I'll be able to finish it in the next few days.

[antony]

Hi @Grotlo

Just working through this on your branch now. We definitely need to fix the duplication, otherwise we're maintaining two completely different components.

I'll see if I can find a way to dedupe it.

[antony]

@Grotlo check your own repo for my first PR. Once you're happy, please merge it into your branch

[Grotlo]

@Grotlo check your own repo for my first PR. Once you're happy, please merge it into your branch

Done! Hey @6eDesign , want to take another look? 😄

[antony]

Hey @6eDesign did you get a chance to look at this?

[6eDesign]

I have not had a chance yet but I would like to. We actually just had our first kiddo, mid-pandemic, and things have been a little crazy. Will try to get to this when things calm down. Thank you both

[antony]

Wow, Congratulations! Don't feel the need to apologise :) focus on your new number 1!

On Sat, 25 Apr 2020 at 19:59, Jonathan Greenemeier ***@***.***> wrote: I have not had a chance yet but I would like to. We actually just had our first kiddo, mid-pandemic, and things have been a little crazy. Will try to get to this when things calm down. Thank you both — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#60 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABVORJ4GLUVEKJP7IZL73TROMXKHANCNFSM4J4IA7IA> .

--

…

________________________________ ꜽ . antony jones . http://www.enzy.org

[antony]

@Grotlo found a small issue with this PR - the on:dateSelected callback doesn't send the start and finish dates, only the start date.

I think it should send from and to if the date is a range.

It also should only trigger when the end date is set, if it is a range.

[antony]

@Grotlo just noticed that this PR also means the calendar doesn't fit on small screens any more (even without range). Not sure what's changed, but will update your PR.

[antony]

Fixed it :)

[dotBits]

seems there's a lot of well-done work already done in here, it would be a pity to not bring these changes in... date range selection is an advanced feature I would need to use as well. Thank you guys, keep it up!

[6eDesign]

I have refactored this component and plan to add a DateRangePicker component in the future. However, to be honest/frank, I am not super happy with the implementation in this PR or the fork Antony created. I will close this PR for now. Feel free to use Antony's fork if it meets your expectations.