Multiple multiday events the second event is not showing on the next row. It is showing in the column after first event colspan ends.

Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7

last update about 2 years ago

Waiting for branch to pass

Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7

last update about 2 years ago

Waiting for branch to pass

🇨🇦Canada Austin986

This is patch of merged branch, meant to be the final patch.

@solideogloria I have merged 3255924-multiple-multiday-events branch into my branch.
Now my branch 3255924-multiday-rendering-fix has all fixes in this thread so far.

🇨🇦Canada Austin986

It looks good to me, but I think an additional review would be helpful, as my use case probably doesn't cover everything.

🇨🇦Canada Austin986

@solideogloria no problem. Take as much time as needed please.

I tested several cases on my own project. (I was motivated to contribute cuz I really need to use this module with Multiday events, and seems no one is working on fix.)
So far so good in my current project scope, so I do not mind if it takes some time to review.

Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7

last update about 2 years ago

Waiting for branch to pass

Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7

last update about 2 years ago

Waiting for branch to pass

Comment over 1 year ago →

🇨🇦Canada Austin986

Status changed to Needs work over 1 year ago

Comment over 1 year ago →

This will need work. In the original code, it has

$item->continuation = $item->getStartDate() < $this->currentDay;
$item->continues = $days > $bucket_cnt;
$item->is_multi_day = TRUE;

These are all dynamic (undeclared) properties, which is deprecated.

Comment over 1 year ago →

Also, the code doesn't follow Drupal coding standards. Boolean values should be in all-caps: FALSE or TRUE

I also can't get all the warnings to go away and have the multi-day stuff still work...

So if someone wants to fix that is this issue or at 🐛 Creation of dynamic properties is deprecated Fixed , go for it. Probably do it here, because the patch here touches all the same code, and there will be merge conflicts if we try to fix it in two commits.

Comment 8 months ago →

🇺🇸United States tregonia

Encountered issues with multiday events pushing the calendar days outside of the container. Applied #25 to Drupal (10.2.7) and the issue is resolved, with no errors found after a 5-minute review.

RTBC +1

I cannot speak to the changes needed; as mentioned in #26-#28.

Comment 5 months ago →

The patch no longer applies after the latest release.

Comment 5 months ago →

🇺🇸United States rhankins

FYI - I have the same issue under 1.0.0-beta3 and the patch wouldn't apply.

solideogloria → changed the visibility of the branch 3255924-multiple-multiday-events to hidden.

solideogloria → changed the visibility of the branch 3255924-multiday-rendering-fix to hidden.

solideogloria → changed the visibility of the branch 3255924-multiday-rendering-fix to active.

Pipeline finished with Canceled

4 months ago

Total: 95s

#387727

I did my best to reroll, but I need some people to test the changes.

Pipeline finished with Failed

4 months ago

Total: 201s

#387731

There were some large blocks of code that had merge conflicts, and I'm not certain of the correct outcome.

Someone else will have to work on this. I literally only use this module for one small thing, so I'm probably just going to give up if nobody else is going to help fix this.

$bucket_index is undefined.

Pipeline finished with Failed

4 months ago

Total: 171s

#387747

First commit to issue fork.

Merge request !51Resolve #3255924 "Rebase" → (Merged) created by karlshea

🇺🇸United States karlshea Minneapolis 🇺🇸

I think some of the difficulty is from lots of merging instead of rebasing—I pushed up a MR with the 3255924-multiday-rendering-fix-merged-v1.1.patch applied and conflicts fixed, then a variable name fix.

It's working for me but I'm not sure I captured all of the fixes that were in the other two branches, I didn't know if some of those changes existed to fix issues or if they were from merge errors.

Pipeline finished with Failed

4 months ago

Total: 165s

#399140

🇺🇸United States karlshea Minneapolis 🇺🇸

I also think trying to fix coding issues here is an exercise in futility, it'll just lead to more rebase/merge issues down the road.

solideogloria → changed the visibility of the branch 3255924-multiday-rendering-fix to hidden.

solideogloria → changed the visibility of the branch 8.x-1.x to hidden.

🇺🇸United States karlshea Minneapolis 🇺🇸

Pushed some CS fixes for code added in this MR.

Pipeline finished with Canceled

4 months ago

Total: 95s

#399153

Pipeline finished with Failed

4 months ago

Total: 170s

#399154

Shorten title more

I'm going to see if the changes work with my site. I have to adjust some custom TypeScript to work with the changes, I think, for me to test it.

I discovered the change in 🐛 Typo in the calendar-month-col twig template Active broke my TS. Once I fixed that, I can test the changes here.

The changes work great, as far as I can test. My test does not cover the entirety of the functionality changes, but I'm happy with what I use and see.

🇨🇦Canada Austin986

Hello,

It seems that drupal/calendar:1.0.0-beta2 already includes this fix.

Unfortunately, I wasn’t able to complete this work, so I want to thank everyone who helped merge parts of my contributions to address the multi-day issues. I really appreciate it!

I’m now wondering what might still be incomplete here. If there’s anything left to do, I’d be happy to contribute further.

Also, I was curious—since some of my work has been incorporated into drupal/calendar:1.0.0-beta2, is there a way to acknowledge or credit that contribution?

Lastly, I no longer see the error in drupal/calendar:1.0.0-beta2, so I’m wondering why this issue hasn’t been closed yet.

Any thoughts?

Look at the changes in the MR. It was broken in 1.0.0-beta3 for me, so the MR needs to be merged.

🇺🇸United States karlshea Minneapolis 🇺🇸

It is definitely still broken in beta3 for me as well.

🇬🇷Greece Savvas.Tsarouchas@intrasoft-intl.com

Hello,

I am trying to update from 1.0.0-beta1 to 1.0.0-beta3.
The version 1.0.0-beta1 was installed using the patch in #5 🐛 Multiple multiday events the second event is not showing on the next row. It is showing in the column after first event colspan ends. Needs review :

Now that I am trying to update to 1.0.0-beta3 with the latest patch in #25 🐛 Multiple multiday events the second event is not showing on the next row. It is showing in the column after first event colspan ends. Needs review , composer fails (it cannot apply the patch.

Is this patch already included in 1.0.0-beta2?

Best regards,
Savvas

No, the patch is not already included. You might have a conflict with another patch you are using. I was able to successfully use the patch with 1.0.0-beta3 without any issues.

Your issue is that #25 is not the most recent patch. You have to use the merge request patch/diff

🇬🇷Greece Savvas.Tsarouchas@intrasoft-intl.com

Dear solideogloria,

Thank you very much for your help. Using the diff you recommended in #57 🐛 Multiple multiday events the second event is not showing on the next row. It is showing in the column after first event colspan ends. Needs review solved the problem.

I successfully updated the module to its latest version.

All the best
Savvas.

Status changed to RTBC 23 days ago

First commit to issue fork.

🇨🇦Canada joelpittet Vancouver

Sorry the code was a bit complex to review and now is way too far to merge, there was quite a few changes. I wonder if the changes can be broken down a bit more discretely so that they are easier to commit?

The merge conflicts are mostly in
src/Plugin/views/style/Calendar.php

The .ddev in .gitignore is no longer needed, and so hope this change wouldn't touch the .gitignore file.

Happy to write a test to ensure this bug doesn't continue to happen, but the changes are quite big from the looks of them, so not sure what to do here...

Unfortunately, it can't be broken down. The functionality is broken unless all that is changed. The other lines are basically to keep the lines fitting on the screen after the changes and pass code review. The .gitignore changes can be left out if desired, but the only way to fix the functionality is to include the complete changes.

The alternative is to find a different way to track whether each day has an event. Currently, the addition of "cell slots" and the completely new functionality for event tracking is what causes the most changes to the file, but it's necessary because the preexisting method doesn't do multiday events correctly.

🇨🇦Canada joelpittet Vancouver

Argh, ok thank for chiming in so fast @solideogloria, I will spend some time testing and rebasing (because I committed the conflicting changes). Hopefully add a test in the process.

The changes are so complicated mostly because the module puts too much functionality into a single file and few functions. If it were broken up into smaller functional pieces with fewer massive loops, it would be easier to change.

(The massive loops make it so a single indentation change or added level causes all the lines to be affected in the patch, making it hard to read.)

🇨🇦Canada joelpittet Vancouver

@solideogloria you are totally right, it's a balance because I am trying not to break existing fixes, also dates in PHP are hard, and the long functions don't help.

Here's a straight reroll as I could get it without fixing any of the coding standards. Attached is a diff of the diffs (as interdiff but it's not) for anybody who might blame the reroll for it not working as it might of before.

There's several phpcs changes that are needed to pass code review.

🇨🇦Canada joelpittet Vancouver

🇨🇦Canada joelpittet Vancouver

@solideogloria Mind giving this a quick try to see if it does what you need? A proper test would be ideal, but honestly a quick manual check is good enough for me to feel okay committing it.

The changes seem to break normal multiday events, even without concurrent ones. I'm going to look and try to see why.

After looking on a whim, the issue is the addition of && $day_no === 1. Please remove that change.

Or, if it was needed from another issue and the rebase, then we need to dig further to determine the proper change.

Personally, I don't see why it was added, as it makes it so that any multiday event on the month calendar that isn't on the first day would be skipped over by the IF statement.

It wasn't the newly merged-in changes that added $day_no, it's been there for the last 9 years, however, it does need to be removed for the existing changes in this issue to work.

My quick guess is that the IF statement needs to be entered for more cases now, in order to track the middle days during a multiday event, so that concurrent multiday events get properly "pushed down", rather than displaying on top of each other.

🇨🇦Canada joelpittet Vancouver

Anybody else watching this issue, feel free to give it a whirl. Thanks very much to @solideogloria for doing that manual test and finding the culprit.

Looks great!

🇨🇦Canada joelpittet Vancouver

System Message

joelpittet → committed 127f1c1d on 8.x-1.x authored by karlshea →

Issue #3255924 by auseidon986, solideogloria, joelpittet, karlshea,...

https://www.drupal.org/project/issues/search?&pr[…]solideogloria&status%5B%5D=Open →

🇨🇦Canada joelpittet Vancouver

@solideogloria Thanks so much for your help getting this through! I always thought (and mentioned in Drupal Slack #drupal-thanks channel yesterday) I should just follow your issues and help you get them over the finish line, as they are always great and well thought through. 🙏

https://www.drupal.org/project/issues/search?submitted=solideogloria&sta... →

Thank you for saying so!

Also, the link above doesn't filter to only my issues, for some reason. Here's a working link:

🇨🇦Canada joelpittet Vancouver

I fixed the link and was targetting any you're "following" 😉 for any lurkers here to do the same!