Fix high/low temp for 'today' period #1319
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
-- What Greg highlighted a simpler way to determine whether or not to only show the low temperature: if there is only 1 'today' period, then it is a night/overnight period and thus should only show the low. Therefore we can remove the more complicated time calc function, since we already have the information.
greg-does-weather
approved these changes
Jun 13, 2024
Comment on lines
+272
to
+277
| $now = \DateTimeImmutable::createFromFormat( | ||
| \DateTimeInterface::ISO8601_EXPANDED, | ||
| "2024-05-07T18:01:00-0500", | ||
| ); | ||
|
|
||
| $this->onLocationRoute(32.778, -96.796); |
There was a problem hiding this comment.
I had to think about this for a minute, but I get it now. Based on how the useOnlyLowForToday flag gets sets below, I was thinking the only thing that mattered was how the data was returned from the API, but then I remembered that we subdivide the daily forecast periods into days based on the current time and the times of the periods.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do? 🛠️
With #1153 we noticed that the 7-day's "today" forecast card, when it's after 6pm, still displayed both the high and low temperatures. We decided that after 6pm and through the "overnight" period, we should only show the low temperature.
This PR implements the change, along with a test.
What does the reviewer need to know? 🤔
It can be hard to visually test this, since we don't have full control of time (ahem, ahem). In the screenshot below, I was able to confirm this is working by checking the weather in Guam at 9:30AM New York time, which is 11:30PM Guam time, and therefore meets the criteria for only showing the low temp.
Shouts to @greg-does-weather for highlighting a simpler way to do this than I originally came up with
Screenshots (if appropriate): 📸