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

Add daily precipitation feature. #98

Merged
merged 3 commits into from
Apr 29, 2024
Merged

Conversation

asandhu3
Copy link
Contributor

I think this adds a valuable piece of data to the forecast section. I've been using this for a while and thought I'd make some proper flags and logic for different units and throw it into a PR if you think it's useful to add. I attempted to add a 16x16 icon beside the number to indicate what it is but it looked a bit cluttered and the very small icons don't render too well IMO.

This is my first exposure to C++ so possibly some issues but feel free to review and offer suggestions!

@lmarzen lmarzen self-requested a review April 29, 2024 16:47
…control over visibility of daily precipition, updated outlook graph to only display precip labels when precip is forecasted
@lmarzen lmarzen merged commit 2bd5ba1 into lmarzen:main Apr 29, 2024
3 checks passed
@lmarzen
Copy link
Owner

lmarzen commented Apr 29, 2024

Merged. @asandhu3 Thank you for your contribution!

I altered your PR to use the same PRECIP_UNITS as everywhere else instead of separate DAILY_PRECIP_UNITS. I also (unrelated to your changes) cleaned up the outlook graph while I was at it to only draw the precipitation y-axis labels when precipitation is forecasted since I got the idea from how you only display daily precip if it is actually forecasted.

You did a great job, made this feature ON by default.

@asandhu3
Copy link
Contributor Author

Awesome, thanks!

@asandhu3
Copy link
Contributor Author

I'm noticing one side affect of your changes that might impact the usefulness of this feature. I remember now why I had separated the units into their own config.

I used this feature to display POP on the graph, and mm on the weekly overview. That way I could at a glance see:

  1. Which days of the week it's expected to rain (and how much), and
  2. At what time of the current day it's expected to rain (on the graph)

By tying those units together it doesn't make that possible.

@lmarzen
Copy link
Owner

lmarzen commented Apr 29, 2024

Whoops, should have discussed before changing it. I'll add that functionality back.

@lmarzen
Copy link
Owner

lmarzen commented Apr 29, 2024

Reverted that change 3fc7cb1

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.

None yet

2 participants