Skip to content

Add absolute dates to weather forecast#2764

Merged
MichMich merged 3 commits into
MagicMirrorOrg:developfrom
oraclesean:patch-1
Jan 7, 2022
Merged

Add absolute dates to weather forecast#2764
MichMich merged 3 commits into
MagicMirrorOrg:developfrom
oraclesean:patch-1

Conversation

@oraclesean

Copy link
Copy Markdown
Contributor
  • 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.
    Add a Boolean configuration option absoluteDates to modules/default/weather to allow weather forecast dates to be formatted as absolute (MON, TUE) or relative (TODAY, TOMORROW).

  • If it includes major visual changes please add screenshots.
    N/A

Add an absolute date option to the weather module's forecast.
Update CHANCEGLOG
@oraclesean oraclesean changed the title Patch 1 Add absolute dates to weather forecast Jan 2, 2022
Comment thread modules/default/weather/forecast.njk
@MichMich

MichMich commented Jan 2, 2022

Copy link
Copy Markdown
Collaborator

Nice idea! @rejas made a valid point.

Also: since this is a new feature, don't forget to send a PR to the docs repo after merge. :)

Set absoluteDates default to false

@oraclesean oraclesean left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Change made to weather.js, thanks for catching that!

@rejas

rejas commented Jan 3, 2022

Copy link
Copy Markdown
Collaborator

Nice. Now it would be awesome if you could add a testcase for that. Think you can do it or do you need help?

@oraclesean

Copy link
Copy Markdown
Contributor Author

@rejas yes I'd appreciate some guidance for a test case. JS is not my ninja area 😄

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

Merging #2764 (0f596d5) into develop (24fccf6) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #2764   +/-   ##
========================================
  Coverage    68.67%   68.67%           
========================================
  Files            8        8           
  Lines          265      265           
========================================
  Hits           182      182           
  Misses          83       83           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24fccf6...0f596d5. Read the comment docs.

colored: false,
showFeelsLike: true
showFeelsLike: true,
absoluteDates: false

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

if this is a property for the forecast, shouldn't this inserted in weatherforecast.js instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought the current and forecast modules were deprecated in favor of the standalone weather module.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

ups, sorry for the noise, was irritated when looking in the tests (theoretically I should know this ...)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

😅👍🏻

@khassel

khassel commented Jan 7, 2022

Copy link
Copy Markdown
Collaborator

wrote a test for this, will provide a PR when this is merged.

@MichMich MichMich merged commit 0e6d40f into MagicMirrorOrg:develop Jan 7, 2022
@MichMich

MichMich commented Jan 7, 2022

Copy link
Copy Markdown
Collaborator

Done!

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.

5 participants