-
-
Notifications
You must be signed in to change notification settings - Fork 28
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
Week view added #62
base: main
Are you sure you want to change the base?
Week view added #62
Conversation
bb140856
commented
Jul 4, 2023
- added week view to Nova Calendar
- views can be enabled in nova-calendar.php config file, selector buttons are located in top right area
- direct links from month view to selected week enabled
- README updated
- Missing comma fix in sample nova-calendar.php config file - Extended class name fix in sample CalendarDataPrivider (should be: AbstractCalendarDataProvider instead of AbstractDataProvider)
Upstream pull
…WeekNumbers to false in nova-calendar.php config file
Week view added
Week view support
config file comments updated to reflect standard behaviour of view render
Modified standard behaviour to render only month view, if no calendarView key provided in config file. If it's empty array, calendarView method in AbstractCalendarDataProvider class returns only ['month'] array. As consequence sanitizeCalendarViews method in CalendarController class simplified. |
Cool, thanks :)
the same goes for the week numbers, by the way.
Updating to this new release should change nothing about the calendar, only make the new features available to the developer, not the end user.
As an open source project I'd prefer to discuss the merge publicly on GitHub, so that other users can chime in and read what the considerations are / were.
Is that ok for you?
Also, let me know if you'd like to be mentioned as a copyright holder in the readme and on-line docs.
You're the first contributor that has made such a big feature 🙌, really spectacular work!
I'm excited about finishing up the release :)
Cheers!
Willem
… On 5 Jul 2023, at 12:20, Bartosz Bujak ***@***.***> wrote:
Modified standard behaviour to render only month view, if no calendarView key provided in config file. If it's empty array, calendarView method in AbstractCalendarDataProvider class returns only ['month'] array. As consequence sanitizeCalendarViews method in CalendarController class simplified.
—
Reply to this email directly, view it on GitHub <#62 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKDYOGOEBWXBLRYJSFTBTLXOU5WHANCNFSM6AAAAAAZ6C5JTE>.
You are receiving this because you were assigned.
|
Week numbers are not displayed on the month view by default.
Hello Team, week numbers are not displayed by default now. Regarding to:
Thanks for warm words and patience. I'm keen on to work on next steps, daily and timeline views. Bartosz |
Just a small heads-up that I've been extensively reviewing the pull request and - great work, Bartosz! I'm compiling a list of things to fix / improve before releasing the feature and have begun implementing a couple of them myself. I'll post the list and an update here later this week or next week at the latest. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reviewed the pull request and did a little work on a few little things already; here's my full feedback.
These changes have already been implemented either by @bb140856 or myself:
✅ shouldShowWeekNumbers should default to false
✅ default calendar view should be month only
✅ move some of the config fields to the calendar data provider so that we can customize on a per-request / per-user basis (and avoid unnecessarily splitting calendar config between the data provider and the config file)
✅ more specific calendar title for week views
✅ view picker buttons as a single grouped row of buttons
First of all, again, great work @bb140856 👏 !
Comments wrt code
- in Week.php; what's with the LAYOUT array and the private properties openingHour, closingHour, timelineInterval, etc.? Seems a bit over-engineered or possibly lingering old code.
- naming of variables openingHour and closingHour could be improved, I'd prefer a more generic name like ...RangeStart or something like that
- naming of variable timelineInterval could be improved, I'd like it to contain the unit (minutes?)
Things that I think I'd want to change before merging and releasing
- 'Flickerless component changing'; right now, switching the view makes the calendar completely disappear for a split second. Month and filter changes don't cause this; the calendar doesn't change until the new data is available. Can we do something equal for view switching?
- Possibly related: the calendar view picker now has to be defined in each individual calendar view, since the component wraps the entire calendar view including the header with calendar controls. Could we switch to a uniform header row, only changing the actual calendar content below it when the view switches? The quick month picker dropdown could make week views simply jump to the week that contains the first day of that month.
- if you activate week view, then remove it from the supported calendar views, on refresh, the system still shows the week view (due to the calendar state stored in localStorage). The back-end should return a response that forces the front-end back to the default calendar view, in case the front-end is requesting a view that is no longer one of the supported views.
Style changes
- make active state of view selector not as strongly accented as the selected filter (less contrast)
- in the week view: the dashed top border line for events that don't start this week should be on the left side of the event and the dashed bottom border line for events that don't end this week should be on the right side of the event (because the direction of days is still left to right).
- I'd like to add month names to week view so that months can always be easily identified. Possibly simply add month name to day numbers
- when multiple events have the same starting time, they are currently shown next to each other, sharing the same vertical space split horizontally. This breaks pretty much the entire interface because you can't read the event details anymore, let alone when you have more than 2 events at the same time. Would it be complicated to switch to events below each other, like we do in the month view, and let the hour slot grow according to its contents if necessary?
For the future
- date format customization of the day column header cell
- we could add an optional marker of configurable color to the current time on today for the week view
I'd be happy to implement almost all of this myself, but the view component switching / splitting it into a static header row with calendar controls from separately swappable calendar views below it is something I'm not too sure how best to implement. How do you see this, @bb140856 ?
I won't be working on the code for the following days so we have time for a discussion if you think any of the points above merit them. I'm open to discussion on all fronts.
Thanks for a great update, looking forward to polishing this off and getting it out there :)!
Latest version is now available on dev branch |
Thank you for reviewing as well as code refactoring and simplifying in some cases. Before I start to update this PR I'd like to agree some issues. LayoutYou are right, naming of variables addresses my context needs, so it should be more self-explaning to all developers,
We can handle this in two ways to address use case described above:
I'm leaving this as a point for discussion. I wonder your opinion on this. Things to be changed prior final release of week view
Style changes
Summarising: @wdelfuego If we can split work on it in the way you proposed - it would be great. Only one disadvantage is that I can work on calendar views Vue components about last week of Aug / first week of Sep. Thanks again, regards! |
Hey @bb140856, thanks for your response! W.r.t. the openingHours/closingHours, I am all for leaving it as is, just a change of variable naming because those are 'interpreted' names, I just want a more neutral name. I'm not fond of showing all hours and auto scrolling to the first event either. In the month view, the calendar shows 6 full weeks but we're only really actively viewing the current month. That is analogus to seeing the full 24 hours but only really actively viewing some time range (eg 8am - 5pm), so perhaps we can use analogous naming, too. In the Month view there are 6 full weeks between startOfCalendar and endOfCalendar, but only 28-31 days between startOfRange and endOfRange. Translated to Week view that could be the full 24h range = startOfDay to endOfDay and the configurable 'office hours' thing could be rangeStartHour to rangeEndHour, something like that. I What I don't understand yet is why in src/View/Week.php there is a const LAYOUT that holds openingHour, closingHour and timelineInterval, and then there's the $openingHour, $closingHour and $timelineInterval private properties. How do these relate to each other? Are they both required? This is not clear to me from the code. W.r.t. showing events that overlap horizontally next to each other, I would very much prefer a different solution. I wouldn't mind growing timeslots, since that is what happens in the month view, too, and while I understand that it doesn't feel maximally elegant, I think it's very preferable to not being able to see any event info at all. W.r.t. "Week view leftovers in local storage"; I would prefer the back-end to be able to respond to a request for week data with month data + a variable indicating that the front-end should switch to month view. That way the back-end decides the state of the calendar after a request, just as is currently the case with other 'dynamic' properties of the calendar view (year/month/filters). I'm all for splitting the work, that's great :)! I'm very busy so I'll wait for your Vue component update around the end of August / beginning of September to get rid of flickering component changing and to make the calendar toolbar a fixed part of the main view, and I'll pick it up from there to get this to release, hopefully somewhere in September. Thanks @bb140856, a pleasure working with you 😃 |
Thanks a lot for working on this @bb140856 and @wdelfuego ! I'm really looking forward to this addition as my client has been asking for a week view for a long time now! |
Yeah, I'm also highly anticipating the release of this feature myself :). Your client is free to sponsor some development of the feature if they want! |
Not that kind of client I'm afraid.. I've never seen a dime from them and they're barely staying afloat themselves. I do dev work for them (Ski Shop) and they give me some free snowboard rentals and the occasional free instructor lesson. If you're ever in Bansko, Bulgaria, I'm pretty sure I can get you some free rentals too after I explain who you are ;-) |
Haha, I'll keep that in mind ⛷ ;)! The feature will come, it's just we both have little time, but it will definitely come! |
Any update on the new awesome weekly view @bb140856 @wdelfuego ? ❤️ |
I will probably have time for this in December. If not, January. |
Hey @wdelfuego. Any news on this package? Anyway I can help? |
Hi @mauveine , thanks for reaching out! The points I mentioned in this comment still need adressing. This is a couple of days work at the most but I have unfortunately not been able to make time for it yet. You could help by 1) making the pull request up to date with the latest changes to the main branch and 2) addressing one or more of the issues mentioned in that comment. Another way to go forward with this is if someone or some company would be willing to sponsor development time on it to make things go faster. |
It looks that this PR is blocked. What do you need ($) to finish to merge it? |
I'd prefer to discuss money matters in private, but I think I'll be able to resolve all outstanding issues and release a week view feature with four or five days of development. You can take a look at the sponsoring page or get in touch with me at willem AT studiodelfuego DOT com for more info. |
Any news on this? Would be very helpful feature! |
I am not sure he is even still working on it. I will probably need to start sponsoring again. |
I am not currently working on this. I've had a very busy year with other things to do. It'd be a valuable update and I'd love to get this feature out there. Unless I either need it in my own projects for myself (which I currently don't), or somebody chooses to address all points mentioned in this comment, or I can do this as a paid job for a sponsor, it is currently difficult for me to take the required time to get it to release. If a portion of the users that use this package commercially can chip in to get some of my development time paid for I'd be happy to make time and get the train rolling again. |
How much you would need to finish this? |
I estimate 4 to 5 days to get to a reliable release; if we can get 4 days sponsored I'll sponsor the 5th myself and make those days in November to get 2.2 with the week view feature released in the first week of December. Half or full development days can be sponsored from the sponsoring page. Doing open source development this way is quite new to me, so please share any feedback, doubts, questions or remarks that could be helpful. |
so 2560 US |