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

feat(date range): added start and end date #60

Closed
wants to merge 76 commits into from

Conversation

Grotlo
Copy link

@Grotlo Grotlo commented Dec 18, 2019

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: bilde bilde

^ This was done with my own test file.

@antony
Copy link
Contributor

antony commented Dec 18, 2019

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
Copy link
Author

Grotlo commented Dec 18, 2019

..., 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!

bilde

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
Copy link
Owner

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
Copy link
Author

Grotlo commented Dec 19, 2019

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
Copy link
Contributor

antony commented Dec 19, 2019

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.

Copy link

@tagptroll1 tagptroll1 left a comment

Choose a reason for hiding this comment

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

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?

src/Components/Datepicker.svelte Outdated Show resolved Hide resolved
src/Components/Datepicker.svelte Outdated Show resolved Hide resolved
src/Components/Datepicker.svelte Outdated Show resolved Hide resolved
@Grotlo Grotlo marked this pull request as ready for review December 20, 2019 10:10
@Grotlo
Copy link
Author

Grotlo commented Dec 20, 2019

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.

@Grotlo Grotlo mentioned this pull request Dec 25, 2019
@antony
Copy link
Contributor

antony commented Jan 5, 2020

@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
Copy link
Owner

6eDesign commented Jan 5, 2020

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
Copy link
Contributor

antony commented Jan 5, 2020

@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
Copy link
Owner

6eDesign commented Jan 5, 2020

@antony
Copy link
Contributor

antony commented Jan 5, 2020

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
Copy link
Author

Grotlo commented Jan 28, 2020

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 Grotlo mentioned this pull request Feb 17, 2020
@Grotlo Grotlo mentioned this pull request Mar 4, 2020
@Grotlo
Copy link
Author

Grotlo commented Mar 6, 2020

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
Copy link
Contributor

antony commented Mar 13, 2020

@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
Copy link
Contributor

antony commented Mar 31, 2020

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
Copy link
Contributor

antony commented Apr 7, 2020

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
Copy link
Contributor

antony commented Apr 7, 2020

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

@Grotlo
Copy link
Author

Grotlo commented Apr 8, 2020

@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
Copy link
Contributor

antony commented Apr 23, 2020

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

@6eDesign
Copy link
Owner

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
Copy link
Contributor

antony commented Apr 25, 2020 via email

@antony
Copy link
Contributor

antony commented Jun 5, 2020

@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
Copy link
Contributor

antony commented Jun 12, 2020

@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
Copy link
Contributor

antony commented Jun 12, 2020

Fixed it :)

@dotBits
Copy link

dotBits commented Nov 27, 2020

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
Copy link
Owner

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.

@6eDesign 6eDesign closed this Sep 19, 2021
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.

Feature: Select Range
6 participants