Skip to content

Conversation

@bughaver
Copy link
Contributor

@bughaver bughaver commented Apr 24, 2025

Hello and thank you for wanting to contribute to the MagicMirror² project!

Please make sure that you have followed these 4 rules before submitting your Pull Request:

  1. Base your pull requests against the develop branch.
  2. Include these infos in the description:
  • Does the pull request solve a related issue? No
  • If so, can you reference the issue like this Fixes #<issue_number>? N/A
  • What does the pull request accomplish? Use a list if needed. Introduce showNextEvent to show/hide next sun event
  • If it includes major visual changes please add screenshots.
    image
  1. Please run npm run lint:prettier before submitting so that
    style issues are fixed.
  2. Don't forget to add an entry about your changes to
    the CHANGELOG.md file.

Note: Sometimes the development moves very fast. It is highly
recommended that you update your branch of develop before creating a
pull request to send us your changes. This makes everyone's lives
easier (including yours) and helps us out on the development team.

Thanks again and have a nice day!

@sdetweil
Copy link
Collaborator

can you explain what this does?

currently the sun is shown as its next event
sunrise if its after sunset and sunset if its after sunrise

what does this new setting do?

@bughaver
Copy link
Contributor Author

bughaver commented Apr 24, 2025

can you explain what this does?

currently the sun is shown as its next event

sunrise if its after sunset and sunset if its after sunrise

what does this new setting do?

This allows the option to disable the next event status on the showSunTimes. Personally I just wanted to see the sun up and sun down without the time till next sun event (like the photo).

Allows this to be configurable
image

@bughaver
Copy link
Contributor Author

Test failing look unrelated (weather module) can the check be run again?

@rejas
Copy link
Collaborator

rejas commented Apr 24, 2025

Will run the tests again.
Can you also add a test for this feature?

@bughaver bughaver changed the title [Feature] Introduce showSunNextEvent to show/hide next sun event [Feature] Introduce showNextEvent to show/hide next sun event Apr 26, 2025
@bughaver
Copy link
Contributor Author

Will run the tests again. Can you also add a test for this feature?

added some tests

@bughaver bughaver changed the title [Feature] Introduce showNextEvent to show/hide next sun event [Feature] Introduce showSunNextEvent to show/hide next sun event Apr 26, 2025
@bughaver
Copy link
Contributor Author

Pr to update the docs MagicMirrorOrg/MagicMirror-Documentation#307

secondsColor: "#888888", // DEPRECATED, use CSS instead. Class "clock-second-digital" for digital clock, "clock-second" for analog clock.

showSunTimes: false,
showSunNextEvent: true,
Copy link
Contributor Author

@bughaver bughaver Apr 26, 2025

Choose a reason for hiding this comment

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

maintaining the existing behavior

Copy link
Collaborator

@rejas rejas Apr 27, 2025

Choose a reason for hiding this comment

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

After looking at the docs PR and how showMoonTimes is configed, I liked the former approach better, sicne it resembled the behaviour of the showMoonTimes config option.
Its not uncommon to have a boolean option with more suboption that also enable it automatically when set.

But thats open for discussion.

Copy link
Contributor Author

@bughaver bughaver Apr 27, 2025

Choose a reason for hiding this comment

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

That's fine. In my original approach I used an object to define sub options, it looks like showMoonTimes uses a single variable with a mixture of strings and booleans. should I follow the showMoonTimes approach or my original? showMoonTimes approach could be less configurable for multiple sub options if in the future someone wants to implement showSunset: false for example but understand if you want to kept it consistent. Let me know what you think is best and I can get to write it up. cc @KristjanESPERANTO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rejas pushed an implementation similar to showMoonTimes

@bughaver bughaver changed the title [Feature] Introduce showSunNextEvent to show/hide next sun event [feature] Introduce showSunNextEvent to show/hide next sun event Apr 26, 2025
@bughaver
Copy link
Contributor Author

@sdetweil @rejas @KristjanESPERANTO any further comments or are you happy to approve?

secondsColor: "#888888", // DEPRECATED, use CSS instead. Class "clock-second-digital" for digital clock, "clock-second" for analog clock.

showSunTimes: false,
showSunNextEvent: true,
Copy link
Collaborator

@rejas rejas Apr 27, 2025

Choose a reason for hiding this comment

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

After looking at the docs PR and how showMoonTimes is configed, I liked the former approach better, sicne it resembled the behaviour of the showMoonTimes config option.
Its not uncommon to have a boolean option with more suboption that also enable it automatically when set.

But thats open for discussion.

@rejas
Copy link
Collaborator

rejas commented Apr 27, 2025

@sdetweil @rejas @KristjanESPERANTO any further comments or are you happy to approve?

just one thing we should clarify. thanks for your patience and your docs PR.

Copy link
Collaborator

@rejas rejas left a comment

Choose a reason for hiding this comment

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

I like this, hopefully th eothers too :-)

@bughaver bughaver changed the title [feature] Introduce showSunNextEvent to show/hide next sun event [feature] Introduce disableNextEvent to hide next sun event Apr 27, 2025
Copy link
Collaborator

@KristjanESPERANTO KristjanESPERANTO left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍 🙂

@bughaver
Copy link
Contributor Author

Thanks @rejas and @KristjanESPERANTO! also seeking approval on the docs changes if you have a moment MagicMirrorOrg/MagicMirror-Documentation#307

@rejas rejas merged commit ff66829 into MagicMirrorOrg:develop Apr 27, 2025
9 checks passed
@bughaver bughaver deleted the showSunNextEvent branch May 11, 2025 07:18
@khassel khassel mentioned this pull request Jun 30, 2025
khassel added a commit that referenced this pull request Jun 30, 2025
## [2.32.0] - 2025-07-01

Thanks to: @bughaver, @bugsounet, @khassel, @KristjanESPERANTO,
@plebcity, @rejas, @sdetweil.

> ⚠️ This release needs nodejs version `v22.14.0 or higher`

### Added

- [config] Allow to change module order for final renderer (or
dynamically with CSS): Feature `order` in config (#3762)
- [clock] Added option 'disableNextEvent' to hide next sun event (#3769)
- [clock] Implement short syntax for clock week (#3775)

### Changed

- [refactor] Simplify module loading process (#3766)
- Use `node --run` instead of `npm run` (#3764) and adapt `start:dev`
script (#3773)
- [workflow] Run linter and spellcheck with LTS node version (#3767)
- [workflow] Split "Run test" step into two steps for more clarity
(#3767)
- [linter] Review linter setup (#3783)
  - Fix command to lint markdown in `CONTRIBUTING.md`
  - Re-activate JSDoc linting and fix linting issues
  - Refactor ESLint config to use `defineConfig` and `globalIgnores`
  - Replace `eslint-plugin-import` with `eslint-plugin-import-x`
- Switch Stylelint config to flat format and simplify Stylelint scripts
- [workflow] Replace Node.js version v23 with v24 (#3770)
- [refactor] Replace deprecated constants `fs.F_OK` and `fs.R_OK`
(#3789)
- [refactor] Replace `ansis` with built-in function `util.styleText`
(#3793)
- [core] Integrate stuff from `vendor` and `fonts` folders into main
`package.json`, simplifies install and maintaining dependencies (#3795,
#3805)
- [l10n] Complete translations (with the help of translation tools)
(#3794)
- [refactor] Refactored `calendarfetcherutils` in Calendar module to
handle timezones better (#3806)
  - Removed as many of the date conversions as possible
- Use `moment-timezone` when calculating recurring events, this will fix
problems from the past with offsets and DST not being handled properly
- Added some tests to test the behavior of the refactored methods to
make sure the correct event dates are returned
- [linter] Enable ESLint rule `no-console` and replace `console` with
`Log` in some files (#3810)
- [tests] Review and refactor translation tests (#3792)

### Fixed

- [fix] Handle spellcheck issues (#3783)
- [calendar] fix fullday event rrule until with timezone offset (#3781)
- [feat] Add rule `no-undef` in config file validation to fix #3785
(#3786)
- [fonts] Fix `roboto.css` to avoid error message `Unknown descriptor
'var(' in @font-face rule.` in firefox console (#3787)
- [tests] Fix and refactor e2e test `Same keys` in
`translations_spec.js` (#3809)
- [tests] Fix e2e tests newsfeed and calendar to exit without open
handles (#3817)

### Updated

- [core] Update dependencies including electron to v36 (#3774, #3788,
#3811, #3804, #3815, #3823)
- [core] Update package type to `commonjs`
- [logger] Review factory code part: use `switch/case` instead of
`if/else if` (#3812)

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Michael Teeuw <michael@xonaymedia.nl>
Co-authored-by: Kristjan ESPERANTO <35647502+KristjanESPERANTO@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Ross Younger <crazyscot@gmail.com>
Co-authored-by: Veeck <github@veeck.de>
Co-authored-by: Bugsounet - Cédric <github@bugsounet.fr>
Co-authored-by: jkriegshauser <joshuakr@nvidia.com>
Co-authored-by: illimarkangur <116028111+illimarkangur@users.noreply.github.com>
Co-authored-by: sam detweiler <sdetweil@gmail.com>
Co-authored-by: vppencilsharpener <tim.pray@gmail.com>
Co-authored-by: veeck <michael.veeck@nebenan.de>
Co-authored-by: Paranoid93 <6515818+Paranoid93@users.noreply.github.com>
Co-authored-by: Brian O'Connor <btoconnor@users.noreply.github.com>
Co-authored-by: WallysWellies <59727507+WallysWellies@users.noreply.github.com>
Co-authored-by: Jason Stieber <jrstieber@gmail.com>
Co-authored-by: jargordon <50050429+jargordon@users.noreply.github.com>
Co-authored-by: Daniel <32464403+dkallen78@users.noreply.github.com>
Co-authored-by: Ryan Williams <65094007+ryan-d-williams@users.noreply.github.com>
Co-authored-by: Panagiotis Skias <panagiotis.skias@gmail.com>
Co-authored-by: Marc Landis <dirk.rettschlag@gmail.com>
Co-authored-by: HeikoGr <20295490+HeikoGr@users.noreply.github.com>
Co-authored-by: Pedro Lamas <pedrolamas@gmail.com>
Co-authored-by: veeck <gitkraken@veeck.de>
Co-authored-by: Magnus <34011212+MagMar94@users.noreply.github.com>
Co-authored-by: Ikko Eltociear Ashimine <eltociear@gmail.com>
Co-authored-by: DevIncomin <56730075+Developer-Incoming@users.noreply.github.com>
Co-authored-by: Nathan <n8nyoung@gmail.com>
Co-authored-by: mixasgr <mixasgr@users.noreply.github.com>
Co-authored-by: Savvas Adamtziloglou <savvas-gr@greeklug.gr>
Co-authored-by: Konstantinos <geraki@gmail.com>
Co-authored-by: OWL4C <124401812+OWL4C@users.noreply.github.com>
Co-authored-by: BugHaver <43462320+bughaver@users.noreply.github.com>
Co-authored-by: BugHaver <43462320+lsaadeh@users.noreply.github.com>
Co-authored-by: Koen Konst <koenspero@gmail.com>
Co-authored-by: Koen Konst <c.h.konst@avisi.nl>
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.

4 participants