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

Compliments module special days config #3465

Closed
WallysWellies opened this issue Jun 13, 2024 · 5 comments
Closed

Compliments module special days config #3465

WallysWellies opened this issue Jun 13, 2024 · 5 comments

Comments

@WallysWellies
Copy link
Contributor

I asked this on the forums and was advised to log a bug.

Platform: RaspberryPi 5 running raspberryPi OS. Current version of MagicMirror software.

When adding config to enable the "special days" feature of the default compliments module it will seemingly add that days comment to the existing array. In my case that array is considerable so special messages such as "Happy Birthday" or "Merry Christmas" etc. are lost in a sea of compliments.

My expectation of this feature would be that on the configured day it shows only the compliments specified in that area of the config. For example:

"....-12-25": ["Happy Christmas!"]

I would expect the only compliment on the 25th December to be "Happy Christmas!". That is not the case though. I am currently adding a line of code in the compliments.js file to empty the array on a special day and only include the configured comments:

// Add compliments for special days
for (let entry in this.config.compliments) {
if (new RegExp(entry).test(date)) {
compliments = []; // Empty the array if it's a special day in config.js
Array.prototype.push.apply(compliments, this.config.compliments[entry]);
}
}

This is not necessarily a "bug" as it may well be the intended behaviour, however I would suggest making this clear in the docs and, if I may be so bold, adding a new config option that allows this behaviour if the user wants it. I am not confident in my coding or GitHub skills to suggest such a change but it seems like it could be quite simple.

@WallysWellies
Copy link
Contributor Author

@sdetweil
Copy link
Collaborator

I agree with your assessment. if you have a specific event list they should be not be combined with the general list.

@rejas
Copy link
Collaborator

rejas commented Jun 16, 2024

I agree also. Do you want to create a PR for that yourself with some help?

@WallysWellies
Copy link
Contributor Author

I would be happy to give it a go! Let me do some testing and reading up on how to GitHub and I’ll get back to you in the next couple of days.

Cheers for the encouragement :)

@WallysWellies
Copy link
Contributor Author

I have done what I think I should have - forked, edited, and submitted a pull request referencing this issue. Please let me know if I have done something wrong.

I have another PR ready to go that updates the documentation with the proposed change but I'll hold fire on that until I know if I am doing this right.

rejas added a commit that referenced this issue Jun 24, 2024
`Fixes #3465`

Add config option `specialDayUnique` that defaults to `false` and causes
special day compliments to be added to the existing compliments array.
Setting this option to `true` will only show the compliments that have
been configured for that day.

---------

Co-authored-by: Veeck <[email protected]>
Co-authored-by: veeck <[email protected]>
Co-authored-by: Karsten Hassel <[email protected]>
khassel added a commit that referenced this issue Jun 30, 2024
## [2.28.0] - 2024-07-01

Thanks to: @btoconnor, @bugsounet, @JasonStieber, @khassel,
@kleinmantara and @WallysWellies.

> ⚠️ This release needs nodejs version >= v20

### Added

- [calendar] Added config option "showEndsOnlyWithDuration" for default
calendar
- [compliments] Added `specialDayUnique` config option, defaults to
`false` (#3465)
- [weather] Provider weathergov: Use `precipitationLast3Hours` if
`precipitationLastHour` is `null` (#3124)

### Removed

- [tests] delete node v18 support (#3462)

### Updated

- [core] Update dependencies including electron to v31
- [core] use node >= v20 (#3462)
- [core] Update `config.js.sample` to use openmeteo as weather provider
which needs no api key
- [tests] Use latest@version of node for `automated-tests.yaml` (#3483)
- [updatenotification] Avoid using pm2 when running in docker container

### Fixed

- [core] Fixed crash possibility if `module: <name>` is not defined and
on `postion: <positon>` mistake (#3445)
- [weather] Fixed precipitationProbability in forecast for provider
openmeteo (#3446)
- [weather] Fixed type=daily for provider openmeteo having no data when
running after 23:00 (#3449)
- [weather] Fixed type=daily for provider openmeteo showing nightly
icons in forecast when current time is "nightly" (#3458)
- [weather] Fixed forecast and hourly weather for provider openmeteo to
use real temperatures, not apparent temperatures (#3466)
- [tests] Fixed e2e tests running in docker container which needs
`address: "0.0.0.0"` (#3479)

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Michael Teeuw <[email protected]>
Co-authored-by: Kristjan ESPERANTO <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Ross Younger <[email protected]>
Co-authored-by: Veeck <[email protected]>
Co-authored-by: Bugsounet - Cédric <[email protected]>
Co-authored-by: jkriegshauser <[email protected]>
Co-authored-by: illimarkangur <[email protected]>
Co-authored-by: sam detweiler <[email protected]>
Co-authored-by: vppencilsharpener <[email protected]>
Co-authored-by: veeck <[email protected]>
Co-authored-by: Paranoid93 <[email protected]>
Co-authored-by: Brian O'Connor <[email protected]>
Co-authored-by: WallysWellies <[email protected]>
Co-authored-by: Jason Stieber <[email protected]>
@khassel khassel closed this as completed Jun 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants