-
Notifications
You must be signed in to change notification settings - Fork 742
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
Fix DaySlotEvents rendering #1531
Conversation
Wouldn't this lead to quite a few more rendering passes - 60 per slot v.s. 60 / MinutesPerSlot (two by default)? Not sure if this is the right approach. |
It would. And I was initially worried about the rendering passes (especially as I had already raised my concerns earlier about an OnSlotRender implementation that would be called 504 times on the We find ourselves between a rock and a hard place. Basically, the current implementation has the shortfalls that mean it can render incorrectly based on a combination of This solution, on initial testing, makes it right, but at the expense of running loop cycles that may or may not be required. I think this is where the back and forth begins! 60 minutes * 24 hours = 1440 passes In the real world, are we going to be booking appointments less than five minutes in duration? Instead of one minute in the loop, we could make it five minutes: That makes it 1440 / 5 = 288 passes (baring in mind that 1440 passes didn't have any detrimental effects in my testing, even on week view, which runs seven times). This could be a parameter on the day and week views called something like that the developer can set as appropriate. What do you think? |
I am against such properties. They usually reveal a major weakness in the implementation - we didn't know what value to pick here so we let you pick it for us. And then the hell starts - what value should I set this to? How do I determine it? Been there, not a nice place to put yourself in. TBH I am still not sure what the wrong behavior is. To me it seems fine - there is an empty space below one of the events but the events still start and end at the right place. |
Yeah, I was just throwing ideas around. I didn't like it myself and have since ditched it. There's definitely a problem with the height of the appointments. If you check this out in the demo `@inject DialogService DialogService <RadzenScheduler @ref=@Scheduler SlotRender=@OnSlotRender style="height: 768px;" TItem="Appointment" Data=@appointments StartProperty="Start" EndProperty="End" <EventConsole @ref=@Console /> @code {
}` the appointment heights are incorrect. I think the change to the height calculation in the PR sorts this out. The problem with the empty space is more to do with immediate interpretation from a GUI point of view. Even though the appointments are mathematically correct, the user automatically assumes that the appointments overlap, when in fact they don't. This is a lot trickier than I first anticipated. |
I think the issue itself doesn't justify the potential performance hit or breaking in other supported cases. You can try addressing the issue in |
I'll give it a bit more thought around those two functions. |
This reverts commit 4982cd1.
Hi @akorchev I'm pretty sure I've sorted this. Just need to do a little more testing before pushing. In the meantime, when rendering the appointment, i.e. - ` @if (item.Start >= start && item.Start <= end)
there's the bit about setting the Many thanks Paul |
The |
@akorchev |
OK, I'm pushing this PR. As you will see, I have ditched the Most of the rendering problems occur when the appointment durations are not multiples of With that in mind, I've achieved this using two main functions as described above.
Is there a column where this appointment has an equal or greater time than the last appointment in the column? If so, add it to that column. If not, create a new column and place it there.
Get all appointments that overlap this appointment where the column is greater. If there is one, the width is calculated up to that appointment, if not, All my tests indicate that this has sorted out the rendering issues described earlier in the comments. Here is some code to paste into the demo site. There are a few days appointments that highlight the rendering issues that occur at the moment and how they render with this new procedure. Because it's all calculated based on the appointments and not the slots (which a user could easily set `@inject DialogService DialogService <RadzenScheduler @ref=@Scheduler SlotRender=@OnSlotRender style="height: 1200px;" TItem="Appointment" Data=@appointments StartProperty="Start" EndProperty="End" <EventConsole @ref=@Console /> @code {
}` It's worth experimenting with overlapping some of the appointments. Also, changing the Speak soon Paul |
I am afraid that's too big of a change. I don't understand the new code and algorithm and thus can't merge it with clear conscience. Could you implement your fix with a minimal change to that file? |
@akorchev I've also just checked the difference file for what I sent. There has been a change in spacing for existing code that has not actually changed, and therefore reported those as changes also. Apologies for that. My next steps will be as follow
|
An aside related to this PR (and previous ones) I don't know how the GitHub network is organized, but would you / Radzen be willing to put their weight behind the following proposal? PR Temporary Comments. It's something I've been thinking about for a while. It could really save some time for both the PR developers and the project owners. Regards Paul |
Unfortunately we don't have any weight when it comes to github feature requests. By the way we don't mind code comments. The issue here is different - the rendering algorithm is completely replaced and not by us. As maintainers we have the responsibility to ensure existing apps don't break in such cases. We also have to support this change - which means we have to fully understand it and be able to fix issues with it. We can't just ping the contribute whenever an issue arises. This is the reason why I undid my fix for this issue - it broke a few cases of existing users. The bigger issue is that this thing doesn't have tests. I couldn't think of a good way to create tests for the layout and describe all edge cases. I think we should abandon this effort for now (unless you can fix it without completely changing the rendering). The effort already spent on it is not on par with its severity. Heck I just tried running it through Github Copilot Chat and ChatGPT for one hour without any success :) I think I am done with it. |
Fully understand @akorchev Due to the fact that it's the existing rendering based on Although I know that the various checks I did against my code (hard coded appointments, click and change appointment time, drag drop, e.t.c.) came out displaying correctly, I'm in no way going to claim that I had covered all possibilities, and it is imperative that you don't introduce any bugs e.t.c., especially on something that hasn't yet caused you guys too much pain. For now, I'll include the Stay well, Atanas Paul P.S. This first picture is rendered as I would expect. After changing the last appointment "Once" to commence at 14:29 (overlapping three other appointments), it correctly moves to one side. But the rendering rest of the appointments has obvious errors. `
@{
} @foreach (var appointment in dayAppointments) @code {
} |
I am sorry but the comments don't improve my understanding of the implementation. Some of the comments explain what the code does but not why. Some examples:
No need for this comment at all.
No information is given about why AppointmentDataRender is needed.
Documents the implementation (which everybody understands) but not what actually true and false means. I won't expect a method called InitializeView to return true and false. And if an Initialize method returns false I would expect that initialization has failed for some reason.
Why are those properties? And why are they public? They should probably be private fields but I am not sure.
Why is Column set to 1? Are columns zero-based or not? If not zero based - why not?
Why is there
What does
What is available2? Could it be just
Couldn't wrap my head around this - call me stupid.
What is the purpose of this?
Again not sure about this one. It seems identical to AppointmentsInSlot though. |
Hopefully, the new comments can be understood and followed through the code. I have always struggled with technical writing. I find it hard to reverse engineer my own code into English, even though it is my first and only language! Also streamlined the code a little. |
Thanks! Thats a lot better. A few issues that I see:
Then use
This will remove the need of a few of the variables and fields used - they will be local in the Layout method. |
Issues resolved -
|
Seems nice! Indeed the rz-focused-state state isn't styled in day view. Something that @yordanov would handle. Thank you for this great contribution! |
Thanks @akorchev 👍 |
A fix to address the following forum posts -
Scheduler Weird behavior when adding 45 min appointments
Scheduler Week View Time Slots Render Issue
Whilst testing the rendering as per multiple events per slot but no time overlap, I noticed that the height of the event wasn't correct.
Fixed that by changing to
var height = Math.Max(1.5, 1.5 * eventEnd.Subtract(eventStart).TotalMinutes / MinutesPerSlot);
This is the same calculation as setting the top of the event, which was always correct. Not sure why these were different.
Admittedly, I haven't carried out extensive tests. Just that appointments don't render side-by-side in same slot when the actually times don't overlap.