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
Homepage and global nav updates #14552
Conversation
46196d6
to
e42ac02
Compare
493e6bf
to
4208ca4
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14552 +/- ##
==========================================
+ Coverage 76.44% 76.78% +0.33%
==========================================
Files 148 152 +4
Lines 7987 8124 +137
==========================================
+ Hits 6106 6238 +132
- Misses 1881 1886 +5 ☔ View full report in Codecov by Sentry. |
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 nits and suggestions but otherwise this set of updates looks good to me. Nice to see Fakespot listed here! 💯
bedrock/base/templates/includes/protocol/navigation/menus/innovation.html
Outdated
Show resolved
Hide resolved
bedrock/base/templates/includes/protocol/navigation/menus/products.html
Outdated
Show resolved
Hide resolved
{% if switch('monitor-plus-banner') and LANG == 'en-US' %} | ||
{{ css_bundle('monitor-banner')}} |
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.
Question: are we keeping this old banner around in case we end up using it, or is it safe to remove? (same for the other bits related to it below).
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.
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.
Checked with Emma, and she suggested we keep the banner code in Bedrock in case the Monitor team asks for it to be used once things are sorted out with the new provider
<section class="c-mission"> | ||
|
||
<!-- Trustworthy AI --> | ||
{% call split( |
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.
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.
Great idea! I added the ftl_has_messages
helper for the Trustworthy AI section, however I've already set the fallbacks for the "What is Mozilla?" section to be the former strings you screenshotted above strings
Oh looks like a functional test might also need updating? https://github.com/mozilla/bedrock/blob/main/tests/functional/test_home.py |
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 LGTM, r+ nice work!
Looks like Dan approved things in the issue too, so I'm gonna merge this: #14543 (comment) |
One-line summary
Some updates came in for the homepage including some extra product info.
Significant changes and points to review
donate_url
helper (made in this PR) and added thefeatured-section
paramIssue / Bugzilla link
Fixes #14543
Testing