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

[weather] add humidity to hourly view, fix spacing error when using UV Index, add config option to hide precipitation entries that are zero #3526

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

OWL4C
Copy link
Contributor

@OWL4C OWL4C commented Aug 29, 2024

Basically the title. Just some existing weather data included into hourly, some config option ("hideZeroes") to hide precipitation when it is zero (this actually shrinks the entire table, removing columns that are completely empty), and add a spacing column to fix the UV Index column.

I don't understand why CHANGELOG.md is broken, i accidentally did my changes to my master, so i just took the current dev branch changelog and copied it into my master changelog, added my edits, and commited. This shouldn't conflict but it does

@sdetweil
Copy link
Collaborator

automerge is turned off..
you have a conflict in the CHANGELOG.md

@OWL4C
Copy link
Contributor Author

OWL4C commented Aug 29, 2024

I hope this fixed it. It may have broken my fork but that is irrelevant, the changes proposed seem fine.

@OWL4C OWL4C marked this pull request as ready for review August 29, 2024 16:39
@rejas
Copy link
Collaborator

rejas commented Aug 30, 2024

Thanks for your PR. Could you attach screenshot sof before and after your changes to this PR here?

@OWL4C
Copy link
Contributor Author

OWL4C commented Aug 30, 2024

Currently:
Screenshot 2024-08-30 at 13 07 35
All functions as mentioned:
Screenshot 2024-08-30 at 13 06 54

However, the need for a spacer is purely the result of a missing css class. I have thus modified the css to include a new (placeholder) class humidity-hourly, as the humidity in the current weather would also be affected by the padding.

.weather .precipitation-amount,
.weather .precipitation-prob,
.weather .humidity-hourly,
.weather .uv-index {
  padding-left: 20px;
  padding-right: 0;
}

What is the naming convention for these classes? As soon as i know what to name the placeholder i will commit a change to remove the spacer from hourly and add the new css class.
(For reference the current css without spacer would look like this:
Screenshot 2024-08-30 at 13 06 28
And the css class with 20px padding looks identical to the 4em whitespace hack)

@OWL4C OWL4C marked this pull request as draft August 30, 2024 13:09
@OWL4C OWL4C marked this pull request as ready for review September 10, 2024 18:17
@OWL4C
Copy link
Contributor Author

OWL4C commented Sep 10, 2024

I just used the placerholder as the actual class name.

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.

sorry for taking so long to review, just some minor things to finish

@@ -21,15 +21,27 @@
{% endif %}
</td>
{% endif %}
{% if config.showHumidity %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

please add the new config options to the defaults object at the top of the file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean the defaults in the weather.js?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, it is a new config property, so it needs to havea default value set, and the defaults:{} object contains those
as documented in the developers guide for modules

modules/default/weather/hourly.njk Outdated Show resolved Hide resolved
modules/default/weather/hourly.njk Outdated Show resolved Hide resolved
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.

3 participants