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

Added Timeline, Daily and Weekly views support #26

Closed

Conversation

bb140856
Copy link
Contributor

Added support for the Timeline (horizontal timescale), Daily and Weekly views

User can customize:

  • time slot interval (only in these 3 views)
  • opening and closing hours (to save screen space and not render night hours if not needed)

The user can navigate between views:

  • from the monthly view directly to the week (by clicking on the week number) or the daily view (by clicking on the day number)
  • from the week view to the daily view (by clicking on the day number)

Tasks to be performed:

  • adding an Event grouping (for example grouping Events by resource such as specialist or location)
  • consideration of the continued use of the DataProvider as injection or rotation versus the Calendar Checker approach
  • data provider refactoring, class is larger than it should be

@wdelfuego
Copy link
Owner

Thanks again for the pull request!

I'll do a thorough code review, but just so you know; I updated a local installation that uses the calendar and without changing anything else I got the following error:

[2022-07-25 01:58:01] local.ERROR: Wdelfuego\NovaCalendar\CalendarDay::withEvents(): Argument #2 ($openingHour) must be of type int, null given, called in /user/Sites/studiodelfuego/nova-calendar/src/DataProvider/Calendar.php on line 222 {"userId":1,"exception":"[object] (TypeError(code: 0): Wdelfuego\\NovaCalendar\\CalendarDay::withEvents(): Argument #2 ($openingHour) must be of type int, null given, called in /user/Sites/studiodelfuego/nova-calendar/src/DataProvider/Calendar.php on line 222 at /user/Sites/studiodelfuego/nova-calendar/src/CalendarDay.php:74)

@bb140856
Copy link
Contributor Author

I was not able to reproduce this issue (installed fresh nova -> added 1.5.2 of nova-calendar -> updated to feature/timeline-daily-weekly-views), however I've changed a little bit definition of default calendar layout.

@wdelfuego
Copy link
Owner

Thanks, that issue is now fixed 🥳 ! I'm still getting weird errors and a broken layout in my existing application though.

Even though it's a breaking change that I'd like to avoid, I changed the parent class to Calendar as the error indicated but still get those errors.

At least part of those errors can be explained by the breaking change of CalendarDay::start to CalendarDay::date.
Start is simply the date, but with the time guaranteed to be 00:00, and I think the changes you made there broke some stuff. At least in existing calendar data providers that rely on it. I also have some unit tests here that are not yet published and about half of them are broken with regard to date logic for the month calendar, I think it might be a result of that change. I'll add the unit tests to the repo so you can run them too, if you want.

I get a semi-functioning interface though (calendar lines are missing, but I see the buttons in the top right and I see the event layouts change) and I must say I'm really impressed by all of your work! I haven't been able to see it in all of its glory yet but it looks like a wonderful addition to this package. Thanks so much!!

I would prefer to keep everything backwards compatible and release this functionality as a 1.* update. That's why I'm testing your pull request on an implementation of the calendar that I'm actually using in production to see if I can integrate these features without breaking my existing functionality. As is, there are some changes that break backwards compatibility though.

Would you agree that we should try to keep this feature completely backwards compatible as a 1.* release or would you think there is a necessity to do a 2.0 release? I hope you want to work with me on that :)!

I'm looking forward to releasing this functionality! Great work, @bb140856 !! 👏

@muhammadsaeedparacha
Copy link
Contributor

@bb140856
bump, would love to see this functionality

@wdelfuego
Copy link
Owner

wdelfuego commented Aug 10, 2022

I'm working with @bb140856 to get this into the package. Don't count on it being available really soon as we've got some delays to work around with. I'm fighting to get this functionality in the package so my expectation is that we could see it within a couple of weeks/months.

Would love to hear feedback from existing users on this feature, so anyone willing to give it a try on a local installation, please do and report :)!

@scramatte
Copy link

@bb140856 and @wdelfuego I would love to have this feature.
Does it possible to sponsor such development to get it early?

Regards

@wdelfuego
Copy link
Owner

wdelfuego commented Aug 14, 2022

Feature sponsoring (ie commercial use of this project by a developer that doesn't use it to implement and/or use calendar views, but who sells (modifications to) the code to other developers to make calendars with) will always be possible between any parties under the AGPL-v3 license. But I'm sure you'll want to use this feature in Nova, too.

I'm open to it, I'll discuss with @bb140856 and let you know if we see any possibilities. This is the first open source project I maintain that has serious amounts of users and contributors so I am learning and open to your thoughts.

Edit: removed interpretation of my own second license because I think I was mistaken. In the end even under the non-AGPL license I don't think there'd be a licensing issue with developers accepting sponsors, as long as the the sponsored features are consequently distributed for free, in public and under the same dual licensing model as the package itself. I might add an extra clause to the license to allow that type of sponsored development explicitly.

@wdelfuego
Copy link
Owner

@bb140856 To be fair, that sounds a little optimistic to me; we have a bit and possibly a lot more than just testing to do.

I haven't seen the new views yet because they won't work on my existing installation due to breaking changes in the code that need fixing, and I haven't done code review yet because I want the code to work first. Depending on those things, we might have a lot of work ahead of us before we're ready for a merge and release.

@wdelfuego wdelfuego added enhancement New feature or request help wanted Extra attention is needed awaiting PR labels Aug 26, 2022
@muhammadsaeedparacha
Copy link
Contributor

This seems to be working fine for me, @wdelfuego Can you dig deeper and tell us where exactly is the breakage? If not fix, then atleast point us in the right direction and I can have one of my team members take a look at it and fix it.

@wdelfuego
Copy link
Owner

wdelfuego commented Sep 3, 2022

@muhammadsaeedparacha I ran into issues on an existing installation due to changes that break backwards compatibility. I didn't review further because I didn't have time yet and I won't start a full code review until the features are at least in a working state. Release 1.6 added unit tests to the package; it's been a while since I ran them against this feature branch but a significant portion failed. That would be one of the first things to fix.

I have been waiting for @bb140856 as the feature developer, they haven't answered my questions about backwards compatibility and I haven't heard from them in a while so I think it makes sense to move ahead with these features without them for now.

For a pull request to be accepted, the code needs to be up to my standards and in line with the architectural vision of the project; I will be very critical about that, but also ready to communicate and contribute to make that happen. I've had to reject your last pull request for several reasons that you didn't respond to; take in mind that this feature branch is much larger and much more complicated than that pull request so it might be a lot more work to get to a point where I'm ready to merge it.

If not fix, then atleast point us in the right direction and I can have one of my team members take a look at it and fix it.

I would prefer if you didn't talk to me as if I have an obligation to help you here. "If not fix then at least..." is the way you would speak to a company when you are a paying client for a product, not the way you speak to a colleague developer who is giving you all of his work for free. Thanks.

@wdelfuego
Copy link
Owner

wdelfuego commented Sep 3, 2022

Also, I am curious to learn which of the three views users are most interested in, as it could be easier to add these one by one than to add all three of them at once. I'd happily consider making my own implementation in order to get to a release faster, but I don't know exactly which feature people are most waiting for.

@bb140856
Copy link
Contributor Author

bb140856 commented Sep 3, 2022

Sorry for the silence. Not going into details, I had some reasons to put on hold my business activities. Give me 2 weeks to dive in to this project, I'll have some time slots to spend them on issues mentioned by @wdelfuego.

Regarding to views preference, in my use case it was crucial to have all of them and to be able to navigate, and/or to have quick eye-view on bookings.

@wdelfuego
Copy link
Owner

Thanks for the update @bb140856, of course anyone who wants to is free to pick up your work and improve on it in the meantime.

I'm still curious: would you agree that we should try to keep this feature completely backwards compatible as a 1.* release or would you think there is a necessity to do a 2.0 release?

@bb140856
Copy link
Contributor Author

bb140856 commented Sep 3, 2022

Thanks for the update @bb140856, of course anyone who wants to is free to pick up your work and improve on it in the meantime.

I'm still curious: would you agree that we should try to keep this feature completely backwards compatible as a 1.* release or would you think there is a necessity to do a 2.0 release?

Fully agree. It should be completely backwards compatible, moreover I see no technical reasons no to keep it.

@wdelfuego
Copy link
Owner

@bb140856 Awesome, thanks for your prompt response!

@muhammadsaeedparacha You are free to go ahead and try to make these features backwards compatible.

The process would roughly be:

  1. make the unit tests succeed again
  2. get these features working on an existing installation that uses all documented customization features in its calendar data provider without having to change anything about the the existing implementation
  3. I will then do a full code review
  4. refactor and finalize the implementation for merge and release.

Let me know if you get to point 2 and I can run some more tests on existing installations or supply calendar data providers to test backwards compatibility.

You are also free to wait until I do steps 1 and 2, which is the option I'd actually prefer, because I would like to combine it with some refactoring of the code underlying the calendar data provider that I have ideas about and it gets rid of communication overhead, but I will need some time because I'm extremely busy now and will be away from work for a couple of weeks soon. I'm pretty confident an official release before Christmas this year should be no problem, faster if we can get the development sponsored (@scramatte?).

In any case, before you start coding, make sure you read the page on contributing that was added to the docs in version 1.6.

I hope this communication today helps advance the release of these features :). Thanks, everyone!

@wdelfuego
Copy link
Owner

I am curious to learn which of the three views users are most interested in

I have created poll #38 to learn more about this. Please respond :)!

@muhammadsaeedparacha since you seem to have a working installation of this feature branch around, would you be able to supply a screenshot of each new view (timeline, daily and weekly) with some events in it, so the poll can make clearer what views we're exactly talking about? Thanks!

@Zen0x7
Copy link

Zen0x7 commented Nov 20, 2022

Hey guys @wdelfuego and @bb140856

There are a way to schedule some meeting and talk about this PR. I can use a weekend to make this works.

@wdelfuego
Copy link
Owner

@SpiritSaint Awesome! Great idea to talk first 👍 .
Drop me an e-mail if you want, you can find my email address in my public event metadata.

@scramatte
Copy link

Hi,

We are very interested into this PR. Please submit screenshots as suggest.
Regards

@wdelfuego wdelfuego mentioned this pull request Jan 11, 2023
@vesper8
Copy link
Contributor

vesper8 commented Jan 22, 2023

This PR looks amazing! I need a week-view in my current client project and I really hope this can be released very soon!

@wdelfuego
Copy link
Owner

Yes, I hope to release the week view as part of the 2.0 release in February :)

@wdelfuego
Copy link
Owner

I am closing this pull request without merging for now, because it needs to be revised to work with the new infrastructural improvements for multiple views in 2.0 that were released today.

I want to add a Week view to this package without adding other views immediately. It'd be great if we could adapt the Week view from this pull request to work with the new back-end logic in 2.0. I'm in touch with @bb140856 to see where he stands on the issue.

@wdelfuego wdelfuego closed this Jun 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting PR enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants