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 WNP128 for Firefox #14720

Merged
merged 8 commits into from
Jul 2, 2024
Merged

Add WNP128 for Firefox #14720

merged 8 commits into from
Jul 2, 2024

Conversation

reemhamz
Copy link
Contributor

@reemhamz reemhamz commented Jun 21, 2024

One-line summary

Adds the Whats New page v128 for Firefox, for both NA and E
Launch date: July 9th 2024

Brief doc

Significant changes and points to review

  • Added WNP for NA and EU
  • NA will have the PiP page and a Nimbus test page
  • EU will have 3 variations:
    • Add-ons page (UK, DE, FR, IT, ES, PL locales) (VERSION A)
    • 2 MoFo donation pages (UK, DE, FR only) (VERSION B & C)
  • We are no longer getting analytics from EU WNPs, so there won't be an analytics JS file for EU
  • Added a new MoFo donation CTA to the bottom of the WNPs going forward (plus legacy WNP)

Issue / Bugzilla link

Fixes #14713

Testing

Local testing

NA

EU

Version A: Add-ons (UK, DE, FR, IT, ES, PL)

Version B: Donate [Fx angle] (UK, DE, FR)

Version C: Donate [MoFo angle] (UK, DE, FR)

Demo servers

NA

EU

Version A: Add-ons (UK, DE, FR, IT, ES, PL)

Version B: Donate [Fx angle] (UK, DE, FR)

Version C: Donate [MoFo angle] (UK, DE, FR)

@reemhamz reemhamz added the WIP 🚧 Pull request is still work in progress label Jun 21, 2024
@reemhamz reemhamz marked this pull request as draft June 21, 2024 04:29
Copy link

codecov bot commented Jun 21, 2024

Codecov Report

Attention: Patch coverage is 65.00000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 77.18%. Comparing base (49da8c3) to head (3f70d0a).
Report is 9 commits behind head on main.

Files Patch % Lines
bedrock/firefox/views.py 65.00% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14720      +/-   ##
==========================================
+ Coverage   77.16%   77.18%   +0.01%     
==========================================
  Files         159      159              
  Lines        8203     8239      +36     
==========================================
+ Hits         6330     6359      +29     
- Misses       1873     1880       +7     

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

janbrasna

This comment was marked as resolved.

@reemhamz reemhamz force-pushed the wnp-128 branch 2 times, most recently from f456cda to 16638c9 Compare June 26, 2024 06:59
@reemhamz reemhamz force-pushed the wnp-128 branch 3 times, most recently from 232c325 to 396566b Compare June 27, 2024 00:08
janbrasna

This comment was marked as resolved.

@reemhamz reemhamz added Needs Review Awaiting code review Review: M Code review time: 1-2 hours and removed WIP 🚧 Pull request is still work in progress labels Jul 1, 2024
@reemhamz reemhamz marked this pull request as ready for review July 1, 2024 23:41
Copy link
Member

@craigcook craigcook left a comment

Choose a reason for hiding this comment

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

Looks good, mostly just some odds and ends that can be cleaned up. Nice work!

@reemhamz reemhamz requested a review from craigcook July 2, 2024 01:52
@reemhamz reemhamz requested a review from craigcook July 2, 2024 11:57
@reemhamz reemhamz removed the Needs Review Awaiting code review label Jul 2, 2024
Copy link
Contributor

@janbrasna janbrasna left a comment

Choose a reason for hiding this comment

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

I think there might be one fluent issue now, otherwise just random nits & bits;)

Comment on lines +14 to +15
{#- This will appear as <meta property="og:description"> which can be used for social share -#}
{% block page_og_desc %}{{ ftl('whatsnew-page-description') }}{% endblock %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Would this be better in whatsnew/base to just set and forget?

(or another base template for that matter — i silently expected the og:description being set to the same as meta description tbh…)

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, og sets self.page_desc():

<meta property="og:description" content="{% filter striptags %}{% block page_og_desc %}{{ self.page_desc() }}{% endblock %}{% endfilter %}">

& meta leaves to the page_desc block:

<meta name="description" content="{% filter striptags %}{% block page_desc %}{% endblock %}{% endfilter %}">

that is never set further (firefox/base or whatsnew/base) so that means empty description in this case.

Which leads me to adding {% block page_desc %}{{ ftl('whatsnew-page-description') }}{% endblock %} to the whatsnew/base should actually fix both the meta and og descriptions for all WNPs.

Copy link
Member

Choose a reason for hiding this comment

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

This is a good suggestion. I think initially when these templates were started we had a different description for each page but somewhere along the way stopped doing that and it's been the same ever since. I'll do a follow-up PR to update the base template(s).

l10n/en/firefox/whatsnew/whatsnew.ftl Show resolved Hide resolved
Comment on lines +25 to +26
<span class="mzp-c-button-icon-start">
<svg width="16" height="16" viewBox="0 0 16 16">
Copy link
Contributor

@janbrasna janbrasna Jul 2, 2024

Choose a reason for hiding this comment

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

BTW I'm not sure how well the image+text renders for you, I'm having better results with additional e.g. line-height: 2 aligning it better inside the button.

@craigcook craigcook merged commit d15d722 into main Jul 2, 2024
6 checks passed
@craigcook craigcook deleted the wnp-128 branch July 2, 2024 17:14
@janbrasna janbrasna mentioned this pull request Jul 17, 2024
24 tasks
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 Review: M Code review time: 1-2 hours
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WNP 128 NA + EU
4 participants