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 wind cardinal direction and update icon #1493

Merged
merged 7 commits into from
Jul 31, 2024

Conversation

greg-does-weather
Copy link
Collaborator

What does this PR do? 🛠️

This PR updates the wind direction icon and adds cardinal and ordinal (N, E, W, S, NE, NW, SE, SW) to wind direction. It also updates the spacing to match the design.

Screenshots (if appropriate): 📸

From current observations:
image

From an hourly table:
image

@greg-does-weather
Copy link
Collaborator Author

@partly-igor Here are some screenshots from my first pass at implementation. The rotation looks okay to my not-very-discriminating eyes. But this is why I'm not a designer. 😂

With the addition of the two-character cardinal directions, the wind becomes two lines in the current observations in a lot of cases. Should we do something about that?

Copy link
Collaborator

@partly-igor partly-igor left a comment

Choose a reason for hiding this comment

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

Good catch on the line breaks. I think the value tds have 2 units of right-padding by default. We could set that to 0 and that should give enough space for the text.

Suggesting an edit to the icon markup that should get us pretty consistent alignment and spacing, and get the arrow to be the right size:

Screenshot of wind icon in table with suggested changes applied

{#
The +180 here is to account for the fact that wind direction
is reported as a FROM direction, but our arrow points in the
TO direction. So we just need to spin it 'round.
#}
<svg role="img" aria-hidden="true" data-wind-direction class="width-2 height-2 margin-left-05 padding-2px" style="transform: rotate({{ wind.direction.angle + 180 }}deg);">
<use xlink:href="{{ "/" ~ directory ~ "/assets/images/spritesheet.svg#wind_arrow_s" }}"></use>
<svg role="img" aria-hidden="true" data-wind-direction class="width-2 height-2 padding-2px" style="transform: rotate({{ wind.direction.angle + 180 }}deg);">
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like putting padding on the actual svg element makes the vector smaller because the padding counts as part of the size, and makes it a little harder to align. So I suggest we warp the svg in a span that we can then add padding and alignment classes to:

Suggested change
<svg role="img" aria-hidden="true" data-wind-direction class="width-2 height-2 padding-2px" style="transform: rotate({{ wind.direction.angle + 180 }}deg);">
<span class="display-flex flex-align-center padding-bottom-2px">
<svg role="img" aria-hidden="true" data-wind-direction class="width-2 height-2" style="transform: rotate({{ wind.direction.angle + 180 }}deg);">

Copy link
Contributor

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?

@greg-does-weather greg-does-weather marked this pull request as ready for review July 31, 2024 16:35
@greg-does-weather
Copy link
Collaborator Author

Oh no, it was totally on purpose. I was just wrong. 😂

Copy link
Collaborator

@partly-igor partly-igor left a comment

Choose a reason for hiding this comment

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

One last missing class for the arrow alignment, otherwise looks good from design perspective now. 🥳 Thank you!

<use xlink:href="{{ "/" ~ directory ~ "/assets/images/spritesheet.svg#wind_arrow_s" }}"></use>
</svg>
<span class="padding-x-05">{{ wind.direction.short }}</span>
<span class="display-flex flex-align-center">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just missing the margin-bottom-2px on the wrapper span

@partly-igor partly-igor self-requested a review July 31, 2024 17:34
igorkorenfeld

This comment was marked as outdated.

Copy link
Collaborator

@partly-igor partly-igor left a comment

Choose a reason for hiding this comment

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

Looks great!

@greg-does-weather greg-does-weather merged commit b71b218 into main Jul 31, 2024
17 checks passed
@greg-does-weather greg-does-weather deleted the mgwalker/1445-wind-direction branch July 31, 2024 18:40
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.

Add wind cardinal direction and update icon
3 participants