-
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
Overhaul Firefox feature pages [fix #11144] #13119
Conversation
To do:
|
78a0d24
to
7485950
Compare
f77833c
to
86fe115
Compare
9387025
to
06094e9
Compare
06094e9
to
e2ec02b
Compare
ea5926a
to
5afc805
Compare
5afc805
to
facada6
Compare
facada6
to
f0b1d7e
Compare
Some test failures, marking do-not-merge until I can sort that out. |
The integration test failures seem unrelated and may just be a timeout on the test runner (it keeps failing on redirect tests, but not on any redirects I added/changed, and fails on different ones each time...). So removing the DNM tag and this is ready for review... but with caution. In a worst case, once it merges we'll see if tests pass on dev and if not, we can fix it there and/or revert the merge and dig deeper. |
f0b1d7e
to
03ed317
Compare
d51d34d
to
46f2827
Compare
Successful test run(!): https://github.com/mozilla/bedrock/actions/runs/5765257018 |
c439805
to
0575c1e
Compare
ed3bbb4
to
2036be0
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.
This was a wild PR. Good job on getting it through the door! 🎉
Main thing that is (potentially) blocking would be the updated strings in adblocker.ftl
file. Check with Peiying if they're going to be deleting strings from their end before we implement this change. If not, then let's update the names of the FTL variables.
Some other notable comments I have are just surrounding visible a11y, where image borders would be nice to add for screenshots + changing body text to black to make it more readable.
bedrock/firefox/templates/firefox/features/picture-in-picture.html
Outdated
Show resolved
Hide resolved
2c56bfc
to
8a2ab03
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.
This looks good to me. However, how come some of the new FTL files have -2023
in their name? I think those would be fine to remove?
r+w(possible)c 🎉
The dated files are ones that already existed but were completely rewritten, so it seemed better to delete the old file and add a brand new one rather than replace every string in an existing file. Some others have been translated but just have a few new strings and we don't want to throw out those translations so I kept the filenames. And still others are brand new pages so maybe those Fluent files should also be dated, just to be consistent. |
cd51c48
to
e682852
Compare
Currently in code-freeze for all-hands. Code freeze lifts August 28th. |
e682852
to
e6c3203
Compare
e6c3203
to
028168a
Compare
One-line summary
Updates Firefox feature pages. Some are rewrites of prior pages, some are brand new, some are reskins of existing pages, and some have been removed entirely.
The original plan was to preserve the old pages as fallbacks and only show the new ones for completed locales, at least for a while. But along the way that idea become untenable because so many are new or total rewrites, and several old pages were so outdated they weren't worth preserving. So this is a clean sweep and fresh start.
Significant changes and points to review
Lots of new strings and some string removal so that stuff needs some careful attention (I'll also ask Peiying for a review when she has time).
Issue / Bugzilla link
#11144
Testing
https://www-demo2.allizom.org/en-US/firefox/features/
http://localhost:8000/firefox/features/