-
Notifications
You must be signed in to change notification settings - Fork 9
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
R7 Today tab implementation #1929
Conversation
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 similar comment
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review looks good! 👍
Code Review Checklist
A checked item is either an assertion that I have tested this item or an indication that I have verified it does not apply to this pull request.
The Basics
- Checks are passing
- I read the code
- I ran the code
Reliability
- error handling exists for unusual or missing values
Accessibility
- New pages have been added to cypress-axe file so that they will be tested with our automated accessibility testing
- Meets WCAG 2.0 AA or 2.1 AA for Section 508 compliance
Device Matrix
- firefox/gecko (renders correctly and user interactions work)
- chrome/chromium/edge (renders correctly and user interactions work)
- safari/webkit (renders correctly and user interactions work)
- web page is readable and usable
- at 480px (mobile)
- at 640px (tablet)
- at 1024px (desktop)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few design edits and a note on the radar ratios but generally looking good. If we want to hide the CMI legend until we figure out a fix, I think that would be fine.
|
||
{% include '@new_weather_theme/partials/satellite.html.twig' %} | ||
<div class="wx-tab-container wx-focus-offset-205" id="today" role="tabpanel" aria-labelledby="current-conditions-tab-button" tabindex="0"> | ||
<div class="grid-container"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add padding-x-0 tablet:padding-x-2
here so that on mobile the cards take up the full width
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eric-gade I think you may have missed this one
{% set message = "There was an error loading the current conditions." | t %} | ||
{% include '@new_weather_theme/partials/uswds-alert.html.twig' with { 'level': "error", body: message } %} | ||
{% else %} | ||
<h3 class="wx-visual-h2 text-normal text-primary-darker padding-x-2 tablet:padding-x-0 margin-top-3 margin-bottom-2"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can change margin-top-3
to margin-top-0
here because the padding on the tab wrapper is duplicating the 3 units of space below the tabs.
<h3 class="wx-visual-h2 text-normal text-primary-darker padding-x-2 tablet:padding-x-0 margin-top-3 margin-bottom-2">{{ "Satellite" | t }}</h3> | ||
</div> | ||
<div class="margin-top-2" wx-outer-radar-container> | ||
<h3 class="wx-visual-h2 text-normal text-primary-darker margin-top-3 margin-bottom-2">{{ "Satellite" | t }}</h3> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After removing the padding on mobile, needs to add padding-x-2 tablet:padding-x-0
so that this heading lines up with the others.
.wx-radar-container .cmi-map { | ||
aspect-ratio: 3/2 !important; | ||
height: auto !important; | ||
width: auto !important; | ||
} | ||
|
||
.wx-radar-container.wx-radar-container__expanded .cmi-map { | ||
aspect-ratio: 3/4 !important; | ||
height: auto !important; | ||
width: auto !important; | ||
} | ||
|
||
@include at-media("tablet") { | ||
.wx-radar-container { | ||
&.wx-radar-container__expanded { | ||
aspect-ratio: 1/1; | ||
aspect-ratio: 1/1 !important; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we talked about, the controls will go back to being absolutely conditioned so we don't have the cyan gap. We're adjusting the aspect ratios to include the controls in order to get it working correctly. I've adjusted the sizes slightly so that the ratios use integer numbers:
Breakpoint | Collapsed | Expanded |
---|---|---|
Mobile | 1/1 | 3/5 |
Tablet | 3/4 | 3/4 |
Desktop | 1/1 | 3/2 |
* Renaming "current" to "today" * Pre-fetching daily forecast data in module * Updating new partials and adding summarized chart options * Updating layout for current cond/today forecast screensizes * Ensuring CC and summary forecast have same height dsktp * satellite/radar layout * Radar layout * Hide arrow for temp chart summary on desktop * Adding expand to radar at desktop breakpoint
* round to two decimals * show qpf chart and table regardless * update tests to require qpf table
Bumps [@types/node](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/node) from 22.7.4 to 22.7.5. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/node) --- updated-dependencies: - dependency-name: "@types/node" dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: James Tranovich <james.tranovich@noaa.gov>
library version bump remove precip table Adding new accordion style and updating radar aspects library bump
Bumps [playwright](https://github.com/microsoft/playwright) from 1.47.2 to 1.48.0. - [Release notes](https://github.com/microsoft/playwright/releases) - [Commits](microsoft/playwright@v1.47.2...v1.48.0) --- updated-dependencies: - dependency-name: playwright dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [eslint-plugin-cypress](https://github.com/cypress-io/eslint-plugin-cypress) from 3.5.0 to 3.6.0. - [Release notes](https://github.com/cypress-io/eslint-plugin-cypress/releases) - [Commits](cypress-io/eslint-plugin-cypress@v3.5.0...v3.6.0) --- updated-dependencies: - dependency-name: eslint-plugin-cypress dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [stylelint](https://github.com/stylelint/stylelint) from 16.9.0 to 16.10.0. - [Release notes](https://github.com/stylelint/stylelint/releases) - [Changelog](https://github.com/stylelint/stylelint/blob/main/CHANGELOG.md) - [Commits](stylelint/stylelint@16.9.0...16.10.0) --- updated-dependencies: - dependency-name: stylelint dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [@playwright/test](https://github.com/microsoft/playwright) from 1.47.2 to 1.48.0. - [Release notes](https://github.com/microsoft/playwright/releases) - [Commits](microsoft/playwright@v1.47.2...v1.48.0) --- updated-dependencies: - dependency-name: "@playwright/test" dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Igor Korenfeld <igor.korenfeld@gsa.gov>
59bbb2e
to
a41dc1f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor things, but otherwise looks good to me.
@@ -1,5 +1,5 @@ | |||
{% set today = weather.daily[0] %} | |||
<h3 class="wx-visual-h2 text-normal text-primary-darker padding-x-2 tablet:padding-x-0 margin-top-3 margin-bottom-2"> | |||
<h3 class="wx-visual-h2 text-normal text-primary-darker padding-x-2 tablet:padding-x-0 margin-top-0 margin-bottom-2"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good for desktop, but realized this needs the 3 units of margin on mobile, otherwise it gets too close to the current conditions section so for this one margin-top-3 desktop:margin-top-0
<span class="font-family-mono text-primary-dark usa-sr-only"> | ||
{{ "Temperature" | t }} | ||
</span> | ||
<div class="position-sticky left-0 display-flex flex-justify padding-top-2 padding-bottom-105 desktop:display-none"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My mistake on this, we do need the arrows on desktop too because it doesn't the max amount of hours (only fits in the afternoon).
&.wx-radar-container__expanded { | ||
aspect-ratio: 1/1; | ||
} | ||
aspect-ratio: 3/4 !important; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mistake on my part, this should have been 4/3 so it's wider than it is tall in the collapsed state. Then 3/4 should be the ratio for the expanded, so then it becomes taller.
What does this PR do? 🛠️
Implements #1918
What does the reviewer need to know? 🤔
There is a bug with the radar "legend" and its sizing that we did not notice before. That will be addressed in #1932
Additionally, there are changes here to make the new "light" version of the accordion. We needed to add two new SVGs with the strokes at the primary color in order to get these to work properly, since the USWDS accordion uses background-image to display the + and - icons
Screenshots (if appropriate): 📸
Mobile
Tablet
Desktop