-
Notifications
You must be signed in to change notification settings - Fork 919
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
Nothing personal updates (fixes #15420) #15445
base: main
Are you sure you want to change the base?
Nothing personal updates (fixes #15420) #15445
Conversation
9dfc175
to
42e2b0c
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15445 +/- ##
==========================================
+ Coverage 78.65% 78.71% +0.06%
==========================================
Files 156 156
Lines 8169 8197 +28
==========================================
+ Hits 6425 6452 +27
- Misses 1744 1745 +1 ☔ View full report in Codecov by Sentry. |
c951830
to
140fd98
Compare
140fd98
to
6cd4ec3
Compare
6cd4ec3
to
591e454
Compare
|
@stephaniehobson could you take a look at this through a GA4/events point of view? |
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.
Just had a look at analytics stuff 👀
- re-exported foxy, heart, and smiley svgs to get rotations from assets - moved stickers from pseudo-elements to inline html to allow animation - added check for dependent animations to run after main animation ends
b18ad9e
to
0437035
Compare
4eabf9e
to
df31bcd
Compare
2912dee
to
ce429f0
Compare
190fe02
to
7a473c7
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.
moved to inline HTML for control over stroke width
} | ||
} | ||
.c-text-title { | ||
@include font-size(34px); |
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.
I think this is the Mozilla font-size scale on Firefox themed page, so the custom property vars were slightly off from Figma in ways that seemed worth overriding for this case
DO NOT MERGE until rebased to single commit after code review & we have green light from Firefox marketing team |
display: flex; | ||
.c-content-layout { | ||
display: grid; | ||
grid-template-columns: 1fr [main-start] auto [main-end aside-start] 1fr [aside-end]; |
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.
I thought this would help avoid more absolute positioning, but I've got to push the aside section over the main section padding anyway, so I'm on the fence about using grid over absolute positioning here. Reviewer's choice!
The mobile browser does not have this function yet. We have helper JS to detect platform and append classes, so we can hide this feature on android and ios. Default will be to show.
686fc24
to
7048d6e
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.
This all looks awesome, really nice work. Just a few minor nits, r+wc 🦊
<main class="c-main"> | ||
<section class="c-things"> | ||
<div class="c-text-section mzp-l-content mzp-t-content-md mzp-t-content-nospace"> | ||
<h2 class="c-text-title">Ok, we admit it: we’re fast, reliable <span class="u-text-uppercase">and</span> private.</h2> |
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.
Suggestion: This span
could be an em
for semantic emphasis as well as presentation (and make it all-caps and non-italic in CSS)
<div class="c-thug-life-gif-wrapper mzp-l-content mzp-t-content-md mzp-t-content-nospace"> | ||
<div class="c-thug-life-gif c-detached-gif"> | ||
<div class="c-things-browser-fox-bush-container mzp-l-content mzp-t-content-md mzp-t-content-nospace"> | ||
<!-- TODO: reduced motion static image --> |
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.
Is this still TODO? Or can this comment be removed?
<aside class="c-aside"> | ||
<a class="c-aside-link" href="https://www.youtube.com/watch?v=dQw4w9WgXcQ" aria-label="Link to a Youtube video" target="_blank" rel="external noopener" data-link-text="Folder"><img src="{{ static('img/firefox/nothing-personal/folder-icon.svg') }}" alt="">Folder <br> <.< </a> | ||
<a class="c-aside-link" href="https://open.spotify.com/playlist/0vvXsWCC9xrXsKd4FyS8kM" aria-label="Link to a Spotify playlist" target="_blank" rel="external noopener" data-link-text="Focus playlist"><img src="{{ static('img/firefox/nothing-personal/music-icon.svg') }}" alt="">focus <br> playlist</a> | ||
<a class="c-aside-link" href="https://www.youtube.com/watch?si=RHt_agrKQEODTPRP&v=F-nFQryDB0s&feature=youtu.be" aria-label="Introducing the new Firefox...jk. It's red pandas" target="_blank" rel="external noopener" data-link-text="MyLife"> |
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.
Nit: can remove some of the additional tracking params from this URL:
<a class="c-aside-link" href="https://www.youtube.com/watch?si=RHt_agrKQEODTPRP&v=F-nFQryDB0s&feature=youtu.be" aria-label="Introducing the new Firefox...jk. It's red pandas" target="_blank" rel="external noopener" data-link-text="MyLife"> | |
<a class="c-aside-link" href="https://www.youtube.com/watch?v=F-nFQryDB0s" aria-label="Introducing the new Firefox...jk. It's red pandas" target="_blank" rel="external noopener" data-link-text="MyLife"> |
<li>Reset password only to be told you can’t use your existing password</li> | ||
</ul> | ||
<details> | ||
<summary><h5>All the things!</h5></summary> |
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 a really nice fallback for non-JS. 👏
} | ||
} | ||
|
||
.c-things-button { |
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.
Should this button also get cursor: pointer;
to help hint at clickability? (Arguably the finger should only be for links but we do apply it on other non-link interactive buttons like form submits, so maybe it's universally understood as "I can click this"?)
One-line summary
Add new content to the
nothing-personal
page.Significant changes and points to review
There should be no change to header styling or functionality.
No change to bottom sticky note functionality.
No missing or broken easter eggs!
Issue / Bugzilla link
#15420
Testing
Figma 10/24 [Mozilla only]: https://www.figma.com/design/QyN625zQl1CWshXiow5NcI/N.A.-Fx-Campaign?node-id=6801-8061&node-type=text
design feedback for reference [Mozilla only]: https://docs.google.com/document/d/1-N_Hu7kpQMI46JsuRLTY0Xs-PVQofwXnq2yCN3P_Pyc/edit?tab=t.0
http://localhost:8000/en-US/firefox/nothing-personal/