-
Notifications
You must be signed in to change notification settings - Fork 913
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
Explore changing /firefox/all/ to be less process intensive. #14324
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14324 +/- ##
==========================================
+ Coverage 77.36% 77.70% +0.34%
==========================================
Files 161 161
Lines 8336 8378 +42
==========================================
+ Hits 6449 6510 +61
+ Misses 1887 1868 -19 ☔ View full report in Codecov by Sentry. |
I've started but I'm not finished and I don't have time to leave notes for someone else to pick it up :/ Fortunately, there's no rush. |
8967869
to
e80f8a5
Compare
e80f8a5
to
435c116
Compare
I pushed my most recent changes.
|
fec1b73
to
889a3dd
Compare
03bba6e
to
a891f3d
Compare
@alexgibson This will change /all to work with a folder structure instead of anchors. Do you think that is likely to break anything? |
There are a few places in bedrock where we point to the old anchors I think, which should be easy enough to update. I have no idea about external sites though. Maybe referrer in GA might point us to any other sites that link to it? |
6ff52e8
to
621e96e
Compare
I added some tests 🎉 And, like any good tests, they have discovered some bugs 🐞 |
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.
Now with the recent rebase even AArch64 Linux Nightlies started working, nice job! 🚀
Right now I'm debugging general layout issues with this branch, pretty site-wide, and can't seem to be finding any culprits in the added styles though:/ I'm constantly having some weird layout issues for this PR when spoofing UAs (or it might be related to OSX versions being frozen in UAstring to 10_15_7 that's also the current support fringe?) like content blinking or disappearing when scrolling in webkit (on unrelated pages like fx/new) or text being dramatically smaller than what it should be in gecko etc. The most visible is ie the CTA alignment or paragraph sizing, anywhere on the site, not just under /firefox or /firefox/all etc.:
bedrock/firefox/templates/firefox/includes/download-unsupported.html
Outdated
Show resolved
Hide resolved
@janbrasna nice find on the missing |
5478e3e
to
c6d2d22
Compare
Ready for review 🎉 |
Oh I see, I was too nosy/early 😇 (but just love this PR and kept an eye on it for a while;)…) One super-minor layout issue I've encountered is at some widths, the chevrons don't play well with longer strings: Screen.Recording.2024-07-23.at.01.47.10.mov(but I guess this Is so minor it can be sorted out later not blocking this going to prod…) The "Release notes" link for Android beta https://www-demo2.allizom.org/en-US/firefox/android/beta/notes/ leads to a version from 2020, that's probably when they stopped publishing them for beta? I can also open an issue for this separately, as this looks like a more meta issue (i.e. should the notes even redirect there, to some v68.x?) I can make it a sad 500 if I feel fancy and put the
Ah that wasn't really me, after weeks of frustration why the layout order doesn't make sense I ran it through all the validators, parsers and formatters I could think of in desperation, eventually pointing out unexpected tokens leading me to areas with something not quite closed or quoted, and the rest was easy;) FYI the nesting fix also resolved the violent flickering in Safari when scrolling as shown on one of the screen captures, so good it was the same cause… 🤞
Good thing is it's from anchors to folders, so any outdated external links to anchors would just land the users at a root "step 0" initial state, still being able to make the selection manually and finish the task. (I can imagine this may get a lightweight progressive JS treatment at a later stage that would only return the options fragments and populate them in-place, instead of full page reloads, that if making use of History API can also be translating the old fragment paths to the new folder structure URLs "as a rewrite"?) BTW the Playwright tests are currently commented out as they liked to time out on the old /all — this new structure now enables writing new tests for it in Playwright too. |
If we need to we can rebase this. I don't have any local changes so it's fine to rebase and force push. |
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.
I can't approve my own PR, but I read through things and it all is looking good to me from the code.
dc8589e
to
7b59852
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.
I'm afraid this misses the functionality to choose from several ESR versions.
The current https://www.mozilla.org/firefox/all/#product-desktop-esr offers the choice of esr-latest vs. esr-next — and most importantly: it's the only place to download it, all the ESR CTAs around the web only link to esr-latest (=115.x), users looking to download esr-next (=128.x) can only find it in /all — and I'm having trouble locating a step where I could make such a choice here (admittedly now only testing demo2, but that's not too old iirc)
@janbrasna Yeah, we missed that. I poked at it for a couple hours but didn't make any progress adding the version. I'll enlist someone else's help (probably not Rob since he's away for 3 weeks). |
c5bd98f
to
d159306
Compare
Promote next over latest to match sys-req and notes
034a008
to
d5cc2ac
Compare
@robhudson FYI I did a force push to rebase. |
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.
I love this. Gives great URLs for direct linking as well. Great work 💯
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.
I took a first-pass and spotted a few issues, but nothing all hopefully minor / easy enough to fix. Excited to see this one land after all this time!
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.
Only observations / naïve questions:
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 look good to me, great work @robhudson @stephaniehobson!
The only remaining thing that's missing from my initial pass is RTL styles for the product icons. Once that's done I think this is good to merge.
r+wc
One-line summary
This branch is an effort to eventually replace
/firefox/all/
to avoid a very heavy paylaod to the browser by breaking the choices into separate pages rather than a single page.Changes
Issue / Bugzilla link
Fix #9845
Fix #14673
Testing
https://www-demo2.allizom.org/en-US/firefox/all/
Analytics:
Mobile:
Localization:
QR codes go to expected App/Play store: