Skip to content

fix: bell icon for events with reminders - #8853

Merged
SebastianKrupinski merged 1 commit into
nextcloud:mainfrom
madbob:issue_4553
Sep 5, 2026
Merged

fix: bell icon for events with reminders#8853
SebastianKrupinski merged 1 commit into
nextcloud:mainfrom
madbob:issue_4553

Conversation

@madbob

@madbob madbob commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Added the bell icon to events having reminders.

I've found that the event.extendedProps.darkText property was calculated only to determine the color of that (broken and missing) icon, that the hexToRGB() function only existed to calculate that value, and that generateTextColorForHex() (the only one calling hexToRGB()) was never used. So I preferred to enforce fill color of SVG icons to var(--color-main-text) (the same color adopted for text) and made the bold move to remove all that redundant code (and related tests).

Fixes #4553

@SebastianKrupinski

Copy link
Copy Markdown
Contributor

Hi @madbob

Thanks for the PR. We'll have to test all these changes.

@SebastianKrupinski SebastianKrupinski left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi,

The changes look good, just some minor things to address.

Comment thread src/fullcalendar/rendering/eventDidMount.js
Comment thread src/fullcalendar/rendering/eventDidMount.js
Comment thread src/fullcalendar/rendering/eventDidMount.js Outdated
@madbob

madbob commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Yes, of course.

Most of changes occurred in the unit tests (meant to test themselves :-P ), other changes are mostly reorganization of existing code.

A note about the hard coded SVG paths: I've modified also the existing ones as them were designed for a different SVG viewport, and I've preferred to normalize all of them. The source of those SVG was not specified, I bet those were just different variants of those already in use from Material Design; I've specified in comments the file from which I got them, for future reference.

@SebastianKrupinski

Copy link
Copy Markdown
Contributor

Most of changes occurred in the unit tests (meant to test themselves :-P ), other changes are mostly reorganization of existing code.

Yes but unit test don't test visual changes

@codecov

codecov Bot commented Sep 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 76.92308% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/fullcalendar/rendering/eventDidMount.js 40.00% 3 Missing ⚠️

📢 Thoughts on this report? Let us know!

Signed-off-by: Roberto Guido <info@madbob.org>
@SebastianKrupinski

Copy link
Copy Markdown
Contributor

/backport to stable6.6

@backportbot backportbot Bot added the backport-request A backport was requested for this pull request label Sep 5, 2026
@SebastianKrupinski

Copy link
Copy Markdown
Contributor

/backport to stable6.5

@SebastianKrupinski
SebastianKrupinski merged commit d7fbb8b into nextcloud:main Sep 5, 2026
32 checks passed
@backportbot backportbot Bot removed the backport-request A backport was requested for this pull request label Sep 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reminder bell icon missing

2 participants