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 to Protocol v.19.0.0 #14084

Merged
merged 7 commits into from
Feb 14, 2024
Merged

Update to Protocol v.19.0.0 #14084

merged 7 commits into from
Feb 14, 2024

Conversation

reemhamz
Copy link
Contributor

@reemhamz reemhamz commented Jan 16, 2024

One-line summary

We've updated Protocol to the latest version of 19.0.0.
There are some significant changes, mentioned here: https://github.com/mozilla/protocol/releases/tag/v19.0.0

Things to highlight:

  • Updated old call-out and call-out compact components to be the new combined Callout component
    • call_out macro changed to callout
    • call_out_compact macro changed to callout_compact
    • New macro parameters have been placed for both macros and can be found in the macros-protocol.html file
  • Removed instanced of the old Picto Card component (remnants only found in /careers/internships template), refer to highlighted Note section below for more info
  • Updated Newsletter component to reflect the new submissions that post directly to Basket

Important

Since the newly rebuilt Callout component does not feature integrated brand logos, I ran into some trouble trying to smoothly add logos on templates that originally had a logo appear with the Callout component (elements were getting wonky), so I created some macro parameters we could use to make adding a brand element easier.

i.e.

{% if brand %}<div class="mzp-t-callout-brand mzp-c-{{ brand_type }} mzp-t-{{ brand_type }}-{{ brand_size }} mzp-t-product-{{ brand_product }}"></div>{% endif %} 

See code changes here: https://github.com/mozilla/bedrock/pull/14084/files#diff-af818836bf22e910767dd7aa9aa7e8b11ee8ae65a1de8561b46d542351e23008R125-R134


Note

I removed the old internships.html template (and its relative internships.scss file) since it's been deprecated for over a year now, and it contained a lot of references to the old Picto Card component we've officially deprecated with this release. I tried to update the component references to Picto, but there were too many references + extra CSS bugs to fix on the page and didn't think it would be useful since we'll probably be looking into updating the content of the page completely.


Testing

You can view a demo of this branch in Demo 5: https://www-demo5.allizom.org/en-US/

Updatedcall-out references to callout

Updated the separatecompact call-out component to a variant of callout

Updated the Newsletter component to highlight integrated Basket posting:

✍🏼 Note: Newsletter updates weren't done individually for pages, but just a few changes on JS files and the static bundle, which in effect changed everything uniformly on all newsletter components around bedrock. I didn't list all of them, but here are some pages that contain the Newsletter component.

💡Make sure to test using something like [email protected]. Avoid using [email protected] as that won't POST properly on localhost

@reemhamz reemhamz marked this pull request as draft January 16, 2024 00:29
@reemhamz reemhamz added the WIP 🚧 Pull request is still work in progress label Jan 16, 2024
@alexgibson
Copy link
Member

alexgibson commented Jan 16, 2024

@reemhamz heads up - it looks like this was missed off the migration notes, but mozilla/protocol#905 is another large-ish change that will require some updates and careful testing in bedrock.

We should hopefully now be able to remove the newsletter JS and tests from bedrock, and pages that feature the newsletter component can now post directly to basket (see code examples in the docs here). We'll need to do a bit of testing to make sure things are still working as expected before merging.

@reemhamz
Copy link
Contributor Author

Thanks for the heads-up @alexgibson! I'll be sure to note those changes down in this PR :)

@reemhamz reemhamz force-pushed the protocol-v19 branch 3 times, most recently from eab4fa7 to d4250bb Compare January 19, 2024 06:41
@reemhamz reemhamz force-pushed the protocol-v19 branch 3 times, most recently from 2baf76d to 83e103e Compare January 25, 2024 04:18
@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 Jan 25, 2024
@reemhamz reemhamz marked this pull request as ready for review January 25, 2024 05:06
@reemhamz reemhamz added Review: L Code review time: 2 hours or more and removed Review: M Code review time: 1-2 hours labels Jan 25, 2024
@alexgibson
Copy link
Member

I used a relay email alias to signed up for a bunch of newsletters on https://www-demo5.allizom.org/en-US/ from various pages, and I can successfully see those subscriptions when I query the email in basket. Things seem good there from a testing perspective! 👍

Copy link
Member

@alexgibson alexgibson left a comment

Choose a reason for hiding this comment

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

I only tested the newsletter logic but it looks good to me, r+wc!

media/js/newsletter/newsletter-init.es6.js Outdated Show resolved Hide resolved
media/js/newsletter/newsletter-init.es6.js Outdated Show resolved Hide resolved
Copy link

codecov bot commented Feb 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9b06488) 77.12% compared to head (c401910) 76.92%.
Report is 20 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14084      +/-   ##
==========================================
- Coverage   77.12%   76.92%   -0.20%     
==========================================
  Files         145      145              
  Lines        7899     7835      -64     
==========================================
- Hits         6092     6027      -65     
- Misses       1807     1808       +1     

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

Copy link
Member

@alexgibson alexgibson left a comment

Choose a reason for hiding this comment

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

Newsletter JS changes look good to me r+ 🥇

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.

Whew, that's a lot! I only spotted a few small issues. Nice work. We should also push this to the run-integration-tests branch before it merges, just to be safe.

bedrock/careers/templates/careers/position.html Outdated Show resolved Hide resolved
media/css/firefox/firstrun/nightly.scss Outdated Show resolved Hide resolved
media/css/firefox/privacy/products.scss Show resolved Hide resolved
@craigcook craigcook merged commit 491ba46 into mozilla:main Feb 14, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review Awaiting code review Review: L Code review time: 2 hours or more
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants