Dealing with "overnight" periods #1017
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
partly-igor
reviewed
Apr 8, 2024
web/themes/new_weather_theme/templates/partials/daily-forecast-list-item.html.twig
Outdated
Show resolved
Hide resolved
partly-igor
reviewed
Apr 8, 2024
web/themes/new_weather_theme/templates/partials/daily-forecast-list-item.html.twig
Outdated
Show resolved
Hide resolved
greg-does-weather
approved these changes
Apr 8, 2024
web/modules/weather_blocks/src/Plugin/Block/Test/EndToEnd/DailyForecast/DailyForecast.php.test
Outdated
Show resolved
Hide resolved
web/themes/new_weather_theme/templates/partials/daily-high-low.html.twig
Show resolved
Hide resolved
web/themes/new_weather_theme/templates/partials/daily-high-low.html.twig
Outdated
Show resolved
Hide resolved
partly-igor
reviewed
Apr 9, 2024
web/themes/new_weather_theme/templates/partials/daily-forecast-list-item.html.twig
Outdated
Show resolved
Hide resolved
3 tasks
|
@partly-igor double check the most recent push, I think I addressed the styling issues properly? |
partly-igor
reviewed
Apr 9, 2024
web/themes/new_weather_theme/templates/partials/daily-forecast-list-item.html.twig
Outdated
Show resolved
Hide resolved
-- What Previously, we were setting the $now value _after_ we had already attempted to filter future daily periods. This prevents us from having any injection of a different $now value for testing purposes and having it filter appropriately. It was also not mapping to the correct timezone, which we have changed.
eric-gade
force-pushed
the
eg-885-overnight-today-period
branch
from
0d9da6a
to
ba6186d
Compare
|
This PR modifies theme Javascript or CSS assets but does not update the theme libraries file. Did you mean to update the appropriate version information in the libraries file? |
|
This PR modifies theme Javascript or CSS assets but does not update the theme libraries file. Did you mean to update the appropriate version information in the libraries file? |
1 task
This pull request was closed.
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? 🛠️
In #885 we discovered that under certain conditions (see below), a user could see three "today" periods for the daily forecast, in addition to the normally expected one or two periods (day/night).
The new period called "overnight" corresponds to the period midnight to 6am for the location. If the user is looking at the weather for a location whose current time (at the location) is between those hours, they will see this additional overnight period. We caught this by looking at Hawaii from the east coast before noon.
This PR addresses the overarching issue (display the information in a better way), while dealing with many under the hood changes / newly discovered bugfixes that make it possible.
What does the reviewer need to know? 🤔
Added Honolulu bundles for testing
We have added some slightly modified bundles from Honolulu -- where the issue was first noticed -- to the testing and e2e example sets. These will be used only in automated testing.
DailyForecastTrait future days filtering and testing
$nowfrom block buildWe have slightly altered how we filter out future daily periods. Previously, the
$nowtime was computed after these filtration, meaning that the current user's time in their own timezone was always being used as the cutoff. This prevented accurate testing of the overnight period issue.Additionally, the
DailyForecastBlock'sbuild()method now takes a$nowDateTime as an optional parameter, and passes it down to the helper methods. The e2e test added in this PR makes use of that.Can't see live in the browser with the bundle alone
Unlike all our other cases of testing with bundles, you will not be able to "see" the overnight period even when looking at the example bundle unless you look between midnight and 6am of the current day. This is because the bundler dates are all relative to the current time, and there is no way from the outside to force the Drupal application to pretend the current time is anything other than what it really is.
Updates to computing high/low
Because the "today" period can have three periods, determining the high/low by day/night is now more ambiguous. Instead, we have adjusted all calculation of high/low to simply take the highest and lowest temperatures returned for each daily period. In most cases the day will still be the high and the night will still be the low. This does not make use of the corresponding hourly data.
Extended periods might only have a "high" value
When there is an "overnight" period, it is not added as an extra period to the number that the API usually provides. This means that the last daily period returned by the API only has a daytime component, the result being we can only display a "high" for that extended day (see third screenshot below).
Closes
#885
Screenshots (if appropriate): 📸
Before:
After:
Extended high-only example: