-
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
footer refresh #14967
footer refresh #14967
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14967 +/- ##
=======================================
Coverage 77.84% 77.84%
=======================================
Files 162 162
Lines 8464 8464
=======================================
Hits 6589 6589
Misses 1875 1875 ☔ View full report in Codecov by Sentry. |
5817c47
to
7a4842a
Compare
af952ed
to
2e7af1e
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.
I know this is still a draft, but I left a few suggestions / comments that I hope might be helpful. Happy to chat further if you have any questions :)
704d781
to
dd41ded
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.
Updates are looking good! I took a closer pass at the CSS / styling and noted a few visual issues. Please let me know if anything I've said doesn't make sense!
bedrock/base/templates/includes/protocol/footer/footer-newsletter.html
Outdated
Show resolved
Hide resolved
bedrock/base/templates/includes/protocol/footer/footer-refresh.html
Outdated
Show resolved
Hide resolved
bedrock/base/templates/includes/protocol/footer/footer-newsletter.html
Outdated
Show resolved
Hide resolved
bedrock/base/templates/includes/protocol/footer/footer-refresh.html
Outdated
Show resolved
Hide resolved
dd41ded
to
876575f
Compare
4c493cc
to
f9f97a8
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.
Updates are looking good!
In addition to the comments below, there are a few integration tests for the footer that will need updating:
- https://github.com/mozilla/bedrock/blob/main/tests/playwright/specs/footer.spec.js
- https://github.com/mozilla/bedrock/blob/main/tests/playwright/specs/a11y/components.spec.js#L131-L177
If we're only targeting en-
languages to begin with, then I'd suggest updating the tests to target the /de/
locale instead of /en-US/
for the time being. Once the footer is enabled in production we can then update the tests later. Does that make sense? Happy to lend a hand here if helpful - just let me know.
bedrock/base/templates/includes/protocol/footer/footer-refresh.html
Outdated
Show resolved
Hide resolved
# License, v. 2.0. If a copy of the MPL was not distributed with this | ||
# file, You can obtain one at https://mozilla.org/MPL/2.0/. | ||
|
||
footer-refresh-find-out-about = Find out about { -brand-name-mozilla } products, initiatives, and more. We’ll never sell your email, even if presented with free pizza for life. |
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.
Issue: this line of copy needs to be changed
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.
Waiting on decision with the new copy at the moment
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 line of copy also needs updating now that it points to the Firefox newsletter, not the Mozilla newsletter.
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 have flagged this for Emma. Hopefully we will get the new copy that is appropriate for the Firefox newsletter soon
I also noticed that the footer styling on all Firefox pages seems to be broken currently (I think because the CSS is only loaded in the Mozilla base template) |
7fa4f50
to
9a866e0
Compare
bedrock/base/templates/includes/protocol/footer/footer-newsletter.html
Outdated
Show resolved
Hide resolved
bedrock/base/templates/includes/protocol/footer/footer-refresh.html
Outdated
Show resolved
Hide resolved
f580b2a
to
e97c211
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.
Other than the outstanding comments I think code-wise, this is in good shape. Nice work! ⭐
bedrock/base/templates/includes/protocol/footer/footer-newsletter.html
Outdated
Show resolved
Hide resolved
e97c211
to
762c4fe
Compare
Looks like this needs a rebase since the home page PR merged |
807d7d7
to
f41b031
Compare
f41b031
to
fad5273
Compare
fad5273
to
79580fb
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.
r+
The bundle issue will be tackled separately in this issue: #15120 |
One-line summary
Significant changes and points to review
Issue / Bugzilla link
#14830
Testing
To enable newsletter sign up form in the footer for a page, add the following: