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

Fix missing events in week view #4431

Merged
merged 1 commit into from
Aug 23, 2022
Merged

Conversation

st3iny
Copy link
Member

@st3iny st3iny commented Aug 19, 2022

If there are too many events on the last day of a week (e.g. Sunday), some might be hidden when the "limit events per view" checkbox is checked.

There already is an ongoing upstream ticket at fullcalendar/fullcalendar#6608. The bug is not yet solved by the FC maintainers but there is a (dirty) quick fix available (ref fullcalendar/fullcalendar#6608 (comment)). Unfortunately, this fix introduces some visual regressions because it changes the ordering of events inside the view. Take a look at my screenshots below (basically events are primarily ordered by title now). If there is a proper solution/fix for the ticket we may revert our changes.

In my opinion, missing events are more severe than having a slightly different event order so I strongly recommend moving on with this.

Thanks to @miaulalala for helping me debug this and finding the mentioned issue.

Before

Screenshot_20220819_122902

After

Screenshot_20220819_122809

Visual regressions

Before

Screenshot_20220819_122831

After

Screenshot_20220819_122749

@st3iny st3iny added this to the v3.5.0 milestone Aug 19, 2022
@st3iny st3iny self-assigned this Aug 19, 2022
@codecov
Copy link

codecov bot commented Aug 19, 2022

Codecov Report

Merging #4431 (b7b43f2) into main (dceecc8) will decrease coverage by 0.01%.
The diff coverage is n/a.

❗ Current head b7b43f2 differs from pull request most recent head c5b5f16. Consider uploading reports for the commit c5b5f16 to get more accurate results

@@             Coverage Diff              @@
##               main    #4431      +/-   ##
============================================
- Coverage     29.42%   29.40%   -0.02%     
  Complexity      330      330              
============================================
  Files           220      220              
  Lines          7702     7700       -2     
  Branches       1019     1017       -2     
============================================
- Hits           2266     2264       -2     
  Misses         5436     5436              
Flag Coverage Δ
javascript 20.49% <ø> (-0.03%) ⬇️
php 68.80% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/components/CalendarGrid.vue 0.00% <ø> (ø)
src/fullcalendar/rendering/eventOrder.js 100.00% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ChristophWurst
Copy link
Member

In my opinion, missing events are more severe than having a slightly different event order so I strongly recommend moving on with this.

Agreed. @jancborchardt wdyt?

@st3iny
Copy link
Member Author

st3iny commented Aug 19, 2022

/backport to stable3.4

@backportbot-nextcloud backportbot-nextcloud bot added the backport-request A backport was requested for this pull request label Aug 19, 2022
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good – thanks for the explanation on the drawbacks. :)

@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Aug 19, 2022
Signed-off-by: Richard Steinmetz <richard@steinmetz.cloud>
@st3iny st3iny force-pushed the fix/noid/missing-events-in-week-view branch from b7b43f2 to c5b5f16 Compare August 23, 2022 08:34
@st3iny st3iny enabled auto-merge August 23, 2022 08:35
@st3iny st3iny merged commit f9ff21e into main Aug 23, 2022
@st3iny st3iny deleted the fix/noid/missing-events-in-week-view branch August 23, 2022 08:43
@backportbot-nextcloud backportbot-nextcloud bot removed the backport-request A backport was requested for this pull request label Aug 23, 2022
st3iny added a commit that referenced this pull request Feb 21, 2024
The underlying issue has been fixed upstream.

This reverts:
- c5b5f16 (#4431)
- 5334250 (#4646)

Signed-off-by: Richard Steinmetz <richard@steinmetz.cloud>
st3iny added a commit that referenced this pull request Mar 18, 2024
The underlying issue has been fixed upstream.

This reverts:
- c5b5f16 (#4431)
- 5334250 (#4646)

Signed-off-by: Richard Steinmetz <richard@steinmetz.cloud>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug Feature: Fullcalendar
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants