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

Week view added #62

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

Week view added #62

wants to merge 26 commits into from

Conversation

bb140856
Copy link
Contributor

@bb140856 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

@wdelfuego wdelfuego self-assigned this Jul 4, 2023
@wdelfuego wdelfuego added enhancement New feature or request in progress labels Jul 4, 2023
@wdelfuego wdelfuego added this to the v2.2 milestone Jul 4, 2023
@wdelfuego wdelfuego self-requested a review July 4, 2023 22:10
@wdelfuego wdelfuego mentioned this pull request Jul 5, 2023
config file comments updated to reflect standard behaviour of view render
@bb140856
Copy link
Contributor Author

bb140856 commented Jul 5, 2023

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.

@wdelfuego
Copy link
Owner

wdelfuego commented Jul 5, 2023 via email

@bb140856
Copy link
Contributor Author

bb140856 commented Jul 5, 2023

Hello Team,

week numbers are not displayed by default now. Regarding to:

  • communication regarding to PR here - I'm absolutely fine,
  • copyrights - yes please, kind, thanks!

Thanks for warm words and patience. I'm keen on to work on next steps, daily and timeline views.

Bartosz

@wdelfuego
Copy link
Owner

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.

Copy link
Owner

@wdelfuego wdelfuego left a 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 :)!

@wdelfuego
Copy link
Owner

Latest version is now available on dev branch

@wdelfuego wdelfuego linked an issue Aug 5, 2023 that may be closed by this pull request
@bb140856
Copy link
Contributor Author

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.

Layout

You are right, naming of variables addresses my context needs, so it should be more self-explaning to all developers,

  1. The openingHour/ closingHour variable addresses me needs not to show events which are out of the standard working hours (business working hours); my intention was, to give developer ability to show events, which are out (before and later) but as minimal as it can be (see pic 1); especially in appointments scheduling cases there is no need to show hours which are out of business, on the other hand - not showing events which are booked during night hours can bee missleading.
Screenshot 2023-08-14 at 12 59 23
  1. The second intention was to visualise which hours (timeslots) are working and which not.

We can handle this in two ways to address use case described above:

  • leave it as it is (with correcting variable naming convetion of course);
  • refactor it to render all hours (00:00 - 23:59) but to scroll the component to the earliest event during the week - in this approach marking working hours would be still needed, I'm afraid of scrolling view programatically because of web view size and compatibility issues I experienced.

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

  1. Flickering views changing - it's obvious, I wonder how I missed this.
  2. Uniform header view - good idea.
  3. Week view leftovers in local storage - sure.

Style changes

  1. View selector (active) - sure
  2. Left dashed border instead of top dashed in all days events - of course, it's a bug left by myself
  3. Month names on week view - agree
  4. Multiple events starting the same time - I agree that UX of such display is not great, I've thought about overlaping events with transparency (like in macOS Calendar App) but have no idea how to do this with CSS only, on the other hand extending time slot and make it not proportional timescale is not what I prefer. MS Teams app for macOS presents events in this way and readability of this is challenging. I think it's the compromise between CSS flexibility and probability of materialising case when more than 2 or 3 events would start the same hour. I'm focused on appointments scheduling use case, when such cases occurs seldom. I thought also about popup view on hover to show more details of event if mouse is over it.
  5. I'll noticed that second calendar view label is not capitalised despite of proper CSS attributes (despite if pure CSS styling or Tailwind used).

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!

@wdelfuego
Copy link
Owner

wdelfuego commented Aug 19, 2023

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 😃

@vesper8
Copy link
Contributor

vesper8 commented Sep 13, 2023

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!

@wdelfuego
Copy link
Owner

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!

@vesper8
Copy link
Contributor

vesper8 commented Sep 13, 2023

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 ;-)

@wdelfuego
Copy link
Owner

Haha, I'll keep that in mind ⛷ ;)!

The feature will come, it's just we both have little time, but it will definitely come!

@vesper8
Copy link
Contributor

vesper8 commented Nov 21, 2023

Any update on the new awesome weekly view @bb140856 @wdelfuego ? ❤️

@wdelfuego
Copy link
Owner

I will probably have time for this in December. If not, January.

@mauveine
Copy link

Hey @wdelfuego. Any news on this package? Anyway I can help?

@wdelfuego
Copy link
Owner

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.

@scramatte
Copy link

It looks that this PR is blocked. What do you need ($) to finish to merge it?

@wdelfuego
Copy link
Owner

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.

@artme-lt
Copy link

Any news on this? Would be very helpful feature!

@jeremiahsherrill
Copy link

I am not sure he is even still working on it. I will probably need to start sponsoring again.

@wdelfuego
Copy link
Owner

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.

@artme-lt
Copy link

How much you would need to finish this?

@wdelfuego
Copy link
Owner

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.
I don't think GitHub supports refunds or conditional sponsoring like Kickstarter so only sponsor if you're sure about it. Of course any amount of sponsored development time will be honored, regardless of the total.

Doing open source development this way is quite new to me, so please share any feedback, doubts, questions or remarks that could be helpful.

@jeremiahsherrill
Copy link

so 2560 US

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Week View Update?
7 participants