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

Update hero with flag animation (fixes #15515 and #15597) #15525

Merged
merged 12 commits into from
Dec 4, 2024

Conversation

maureenlholland
Copy link
Collaborator

@maureenlholland maureenlholland commented Nov 19, 2024

One-line summary

Add waving, blinking, bouncing flag animation on homepage hero.
Animation play state will respect user's motion preference by default.
There will also be a play/pause button to change the default play state.

Based on this codepen: https://codepen.io/slightlyoffbeat/pen/LYwwpgj

  • I used an AI to write some of this code.

Significant changes and points to review

The animation is hidden behind a switch: M24_HERO_ANIMATION
When the switch is off, you will get the no js experience

Play/pause button on desktop and mobile Figma [Mozilla only]

Expected behaviour

no js: by default animation does not run

user motion preference:

  • if reduced, animation does not run on load
  • if no preference, animation runs on load

button click

  • if playing, animation pauses and button text changes to "Play Animation"
  • if paused, animation plays and button text changes to "Pause Animation"

Issue / Bugzilla link

#15515
#15597
#15640

Testing

M24_WEBSITE_REFRESH must be on

Use ./manage.py waffle_switch --create M24_HERO_ANIMATION on to test active switch

  • no js works as described
  • prefers reduced motion works as described
  • no preference for reduced motion works as described
  • mobile animation button is icon only
  • desktop animation button is icon + text
  • animation button allows user to control play/pause functionality
  • flag is under text on mobile

@maureenlholland maureenlholland force-pushed the animate-flag-15515 branch 4 times, most recently from 0962ab1 to 79af51e Compare November 20, 2024 21:50
Copy link

codecov bot commented Nov 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.04%. Comparing base (8127ca3) to head (cf081a0).
Report is 18 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #15525      +/-   ##
==========================================
- Coverage   79.05%   79.04%   -0.01%     
==========================================
  Files         159      159              
  Lines        8331     8329       -2     
==========================================
- Hits         6586     6584       -2     
  Misses       1745     1745              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alexgibson
Copy link
Member

alexgibson commented Nov 21, 2024

Adding a note here that I had some success fixing the sub-pixel tearing by adding stroke="#000" stroke-width="1" to each shape and removing shape-rendering="crispEdges". Worth some more testing, but it seems worth exploring.

Codepen: https://codepen.io/alxgbsn/pen/GRVbbpO

@slightlyoffbeat
Copy link
Collaborator

Here is my attempt for the evening.

  • using SVG filter to fix tearing
  • fixing blink timing

https://codepen.io/slightlyoffbeat/pen/LYwwpgj

@slightlyoffbeat
Copy link
Collaborator

Another attempt

  • removes feMorphology
  • adds stroke width of .35
  • adds vector-effect: non-scaling-stroke;

https://codepen.io/slightlyoffbeat/pen/XWvvXpg

Seems to work the best of what I've tried so far.

@maureenlholland maureenlholland force-pushed the animate-flag-15515 branch 3 times, most recently from ac0c922 to a21cf2e Compare December 2, 2024 20:53
margin-bottom: $spacer-xl;
}

.m24-c-flag-cta {
margin-bottom: 0;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removes default p spacing

background-color: $m24-color-medium-gray;
color: $m24-color-white;
font-family: $primary-font;
font-size: 0.75rem;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

non-variable values used when there's no change in value across viewport widths

border-radius: 50%;
display: block;
margin-bottom: $spacer-md;
margin-inline-start: auto;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

logical properties have pretty good support now, so I decided to use them as enhancement here instead of bidi mixin

@maureenlholland maureenlholland changed the title Initial animation testing @maureenlholland Update hero with flag animation (fixes #15515) Dec 2, 2024
@maureenlholland maureenlholland changed the title @maureenlholland Update hero with flag animation (fixes #15515) Update hero with flag animation (fixes #15515) Dec 2, 2024
@maureenlholland maureenlholland force-pushed the animate-flag-15515 branch 2 times, most recently from 8f8d5f5 to 97256ca Compare December 3, 2024 13:55
<img src="{{ static('img/home/2024/hero-symbol-white.svg') }}" alt="" width="216" height="243">
</div>
<section class="m24-c-flag" data-animation-running="false">
<button class="m24-c-flag-button js-animation-button" aria-live="assertive">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

went for assertive here as the text change might be confusing if not announced immediately after button click

@maureenlholland maureenlholland marked this pull request as ready for review December 3, 2024 14:01
@maureenlholland maureenlholland requested a review from a team as a code owner December 3, 2024 14:01
@@ -13,6 +13,8 @@ m24-home-page-desc = Did you know? { -brand-name-mozilla } — the maker of { -b

## Intro

m24-home-pause-animation = Pause animation
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

question for reviewer: should these be in a more generic UI ftl file?

Copy link
Member

Choose a reason for hiding this comment

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

Good idea. That's a global file so the string would be available everywhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks, will update!

@maureenlholland maureenlholland changed the title Update hero with flag animation (fixes #15515) Update hero with flag animation (fixes #15515 and #15597) Dec 3, 2024
@maureenlholland maureenlholland added Needs Review Awaiting code review Review: XS Code review time: up to 30min Frontend HTML, CSS, JS... client side stuff Refresh 🦖 Work related to the 2024 site refresh labels Dec 3, 2024
@maureenlholland
Copy link
Collaborator Author

maureenlholland commented Dec 3, 2024

Feedback

  • remove the wink and bounce animations
  • change the flag (and pole)) from white to zilla green var(--color-green)
  • Align the "pause animation" text with the pause icon
  • put the animation behind a switch with a unique name (this commit accidentally got amended into the green update)

@maureenlholland maureenlholland force-pushed the animate-flag-15515 branch 2 times, most recently from 13c7659 to 104c2a2 Compare December 3, 2024 19:15
@maureenlholland maureenlholland requested a review from a team as a code owner December 3, 2024 19:15

.m24-c-flag-button {
$font-size: 0.75;
all: unset;
Copy link
Contributor

Choose a reason for hiding this comment

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

This removed the focus ring. Can you add it back please? Ideally with the native focus ring . But if that is finiky any kind of outline would be a good addition.

media/css/m24/flag.scss Outdated Show resolved Hide resolved
@@ -20,6 +20,8 @@ ui-show-all = Show All
ui-hide-all = Hide All
ui-learn-more = Learn more
ui-view = View
ui-pause-animation = Pause animation
Copy link
Contributor

Choose a reason for hiding this comment

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

Praise: Thanks for making these global.

stephaniehobson and others added 2 commits December 4, 2024 10:32
Co-authored-by: Jan Brasna <1784648+janbrasna@users.noreply.github.com>
Copy link
Contributor

@stephaniehobson stephaniehobson left a comment

Choose a reason for hiding this comment

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

R+

@stephaniehobson stephaniehobson merged commit 7f84d19 into mozilla:main Dec 4, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Frontend HTML, CSS, JS... client side stuff Needs Review Awaiting code review Refresh 🦖 Work related to the 2024 site refresh Review: XS Code review time: up to 30min
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants