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.

Sign up for GitHub

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

Jump to bottom

Add products to product list page #14407

Merged
merged 12 commits into from May 1, 2024
Merged

Add products to product list page #14407

merged 12 commits into from May 1, 2024

Conversation

stephaniehobson
Copy link
Contributor

@stephaniehobson stephaniehobson commented Apr 3, 2024 •
edited

One-line summary

Add products to product list page and convert to base Protocol.

Significant changes and points to review

  • add MDN Plus, Fakespot, and Thunderbird
  • add corresponding strings and remove obsolete ones
  • move MDN Plus logo from privacy folder to generic folder

Issue / Bugzilla link

Fix #14220 (the epic for the issues below)
Fix #14290
Fix #14455
Fix #14456
Fix #14457

Testing

https://www-demo9.allizom.org/en-US/products/

  • test all links
  • test conditional formatting for ios and android
  • look for spelling errors

@stephaniehobson
Copy link
Contributor Author

@craigcook even if you don't do the full code review, could you please check for spelling errors in the fluent strings 🙏

@stephaniehobson stephaniehobson added Needs Review Awaiting code review Review: XS Code review time: up to 30min labels Apr 3, 2024
janbrasna
janbrasna reviewed Apr 4, 2024
View reviewed changes
Copy link
Contributor

@janbrasna janbrasna left a comment •
edited

Choose a reason for hiding this comment

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

Oh this is such an improvement over the screenshots! Love it. 🚀

Finally got to testing properly on Android and iOS devices and not just simulators, and it works as designed. 💟 With the usual issue that if you wind up in de locale but don't have a de appstore, you get an error about not being able to download Klar and no way to force Focus, but that's unrelated to the PR…

At first I thought I didn't have the right bundles loaded as the page was linear, but then noticed the three column grid only kicks in at 1024px (the old page had a fallback 2-column layout between 768–1024px). The two column fallback seems to work well with this content, would it be something worth considering? PoC: janbrasna@78bd648

Aside from that, only quick nits noticed:

bedrock/products/templates/products/landing.html Outdated Show resolved Hide resolved
bedrock/products/templates/products/landing.html Outdated Show resolved Hide resolved
bedrock/products/templates/products/landing.html Show resolved Hide resolved
@janbrasna janbrasna mentioned this pull request Apr 8, 2024
Closed
reemhamz
reemhamz requested changes Apr 9, 2024
View reviewed changes
Copy link
Contributor

@reemhamz reemhamz left a comment

Choose a reason for hiding this comment

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

Nice job with this! It looks much cleaner and more concise than before.
There are a few suggestions, nits and one nit that's blocking. Some other comments from Jan are recommended to follow too. r+wc

media/img/logos/thunderbird/logo.svg Outdated Show resolved Hide resolved
media/css/products/landing.scss Outdated Show resolved Hide resolved
media/css/products/landing.scss Outdated Show resolved Hide resolved
bedrock/products/templates/products/landing.html Outdated Show resolved Hide resolved
bedrock/products/templates/products/landing.html Outdated Show resolved Hide resolved
@janbrasna janbrasna mentioned this pull request Apr 9, 2024
Closed
@reemhamz reemhamz removed the Needs Review Awaiting code review label Apr 15, 2024
janbrasna
janbrasna reviewed Apr 15, 2024
View reviewed changes
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.

Only cosmetics before launch, making sure all the external links & buttons have the right attribution:

bedrock/products/templates/products/landing.html Outdated Show resolved Hide resolved
bedrock/products/templates/products/landing.html Outdated Show resolved Hide resolved
bedrock/products/templates/products/landing.html Outdated Show resolved Hide resolved
bedrock/products/templates/products/landing.html Show resolved Hide resolved
@reemhamz
Copy link
Contributor

Is this ready for re-review or are you still working on the changes?

@stephaniehobson
Copy link
Contributor Author

Still working on the unresolved conversations.

@janbrasna janbrasna mentioned this pull request Apr 23, 2024
Merged
stephaniehobson and others added 10 commits April 23, 2024 14:44
@stephaniehobson
Add products to product list page
07c60b3 
- add MDN Plus, Fakespot, and Thunderbird
- add corresponding strings and remove obsolete ones
- move MDN Plus logo from privacy folder to generic folder
@stephaniehobson
(fix) remove old images
7f8e62c
@stephaniehobson
(fix) use existing thunderbird logo, line up logos
7223810
@stephaniehobson
(fix) revert changes to relay logo
748d976
@stephaniehobson
(fix) remove logo component css
8ab517b
@stephaniehobson
(fix) use universal import for picto
6a2e915
@stephaniehobson @janbrasna
Update l10n/en/firefox/products.ftl
52f6469 
(fix) match fakespot punctuation to pocket

Co-authored-by: Jan Brasna <1784648+janbrasna@users.noreply.github.com>
@stephaniehobson
(fix) referrals and string fallback
9b9c05a
@stephaniehobson
(fix) button icon
2a6b6bd
@stephaniehobson
(fix) add download icon to thunderbird CTA
d05e119
@stephaniehobson
Copy link
Contributor Author

Ready for re-review.

@reemhamz reemhamz self-requested a review April 23, 2024 22:24
reemhamz
reemhamz requested changes Apr 30, 2024
View reviewed changes
Copy link
Contributor

@reemhamz reemhamz left a comment

Choose a reason for hiding this comment

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

Nice going with patching things up!
I just have a few more nits and suggestions that should be easy to fix up, and it'll be good to go 👍🏼

bedrock/products/templates/products/landing.html Outdated Show resolved Hide resolved
bedrock/products/templates/products/landing.html Outdated Show resolved Hide resolved
bedrock/products/templates/products/landing.html Outdated Show resolved Hide resolved
media/img/logos/relay/logo.svg Outdated Show resolved Hide resolved
media/img/logos/mdn/mdn-plus-logo.svg Outdated Show resolved Hide resolved
media/img/logos/fakespot/logo.svg Outdated Show resolved Hide resolved
media/css/products/landing.scss Show resolved Hide resolved
@stephaniehobson
Copy link
Contributor Author

Updated.

@codecov Codecov
Copy link

codecov bot commented Apr 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.45%. Comparing base (6fee488) to head (fbdcd4e).
Report is 10 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #14407   +/-   ##
=======================================
  Coverage   76.44%   76.45%           
=======================================
  Files         148      148           
  Lines        7978     7979    +1     
=======================================
+ Hits         6099     6100    +1     
  Misses       1879     1879

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

reemhamz
reemhamz approved these changes May 1, 2024
View reviewed changes
Copy link
Contributor

@reemhamz reemhamz left a comment

Choose a reason for hiding this comment

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

w00t! r+

@reemhamz reemhamz merged commit 911e909 into main May 1, 2024
7 checks passed
@reemhamz reemhamz deleted the products branch May 1, 2024 00:58
reemhamz pushed a commit that referenced this pull request May 1, 2024
@stephaniehobson @janbrasna @reemhamz
Add products to product list page (#14407)
0670fac 
* Add products to product list page

- add MDN Plus, Fakespot, and Thunderbird
- add corresponding strings and remove obsolete ones
- move MDN Plus logo from privacy folder to generic folder

* (fix) remove old images

* (fix) use existing thunderbird logo, line up logos

* (fix) revert changes to relay logo

* (fix) remove logo component css

* (fix) use universal import for picto

* Update l10n/en/firefox/products.ftl

(fix) match fakespot punctuation to pocket

Co-authored-by: Jan Brasna <1784648+janbrasna@users.noreply.github.com>

* (fix) referrals and string fallback

* (fix) button icon

* (fix) add download icon to thunderbird CTA

* (fix) svgs

* (fix) dl icon to dl button

---------

Co-authored-by: Jan Brasna <1784648+janbrasna@users.noreply.github.com>
@craigcook craigcook mentioned this pull request May 1, 2024
Merged
reemhamz pushed a commit that referenced this pull request May 1, 2024
@stephaniehobson @janbrasna @reemhamz
Add products to product list page (#14407)
23d12e6 
* Add products to product list page

- add MDN Plus, Fakespot, and Thunderbird
- add corresponding strings and remove obsolete ones
- move MDN Plus logo from privacy folder to generic folder

* (fix) remove old images

* (fix) use existing thunderbird logo, line up logos

* (fix) revert changes to relay logo

* (fix) remove logo component css

* (fix) use universal import for picto

* Update l10n/en/firefox/products.ftl

(fix) match fakespot punctuation to pocket

Co-authored-by: Jan Brasna <1784648+janbrasna@users.noreply.github.com>

* (fix) referrals and string fallback

* (fix) button icon

* (fix) add download icon to thunderbird CTA

* (fix) svgs

* (fix) dl icon to dl button

---------

Co-authored-by: Jan Brasna <1784648+janbrasna@users.noreply.github.com>
alexgibson pushed a commit to alexgibson/bedrock that referenced this pull request May 1, 2024
@stephaniehobson @janbrasna @alexgibson
Add products to product list page (mozilla#14407)
bc41f8e 
* Add products to product list page

- add MDN Plus, Fakespot, and Thunderbird
- add corresponding strings and remove obsolete ones
- move MDN Plus logo from privacy folder to generic folder

* (fix) remove old images

* (fix) use existing thunderbird logo, line up logos

* (fix) revert changes to relay logo

* (fix) remove logo component css

* (fix) use universal import for picto

* Update l10n/en/firefox/products.ftl

(fix) match fakespot punctuation to pocket

Co-authored-by: Jan Brasna <1784648+janbrasna@users.noreply.github.com>

* (fix) referrals and string fallback

* (fix) button icon

* (fix) add download icon to thunderbird CTA

* (fix) svgs

* (fix) dl icon to dl button

---------

Co-authored-by: Jan Brasna <1784648+janbrasna@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janbrasna janbrasna janbrasna left review comments

@reemhamz reemhamz reemhamz approved these changes

Assignees
No one assigned
Labels
Review: XS Code review time: up to 30min
Projects
None yet
Milestone
No milestone
3 participants
@stephaniehobson @reemhamz @janbrasna