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

Fix Bug 1430027 - On the download page, for Linux, highlight the 64-bit version (to limit the number of 32 downloads) #5432

Merged
merged 1 commit into from Feb 28, 2018

Conversation

kyoshino
Copy link
Contributor

@kyoshino kyoshino commented Feb 2, 2018

Description

Issue / Bugzilla link

Bug 1430027 - On the download page, for Linux, highlight the 64-bit version (to limit the number of 32 downloads)

Testing

  • Demo server is available
  • The change should not affect download pages other than Firefox for desktop.

@flodolo
Copy link
Contributor

flodolo commented Feb 12, 2018

l10n: there are 2 new strings: Recommended and Legacy.

Are the strings final here?

I see they're not from the bug. Given that we can't use tags, the sooner we agree on the text and expose it for translation, the better. We should also keep the PR on hold for a few days, to allow at least the top locales to catch up.

@kyoshino
Copy link
Contributor Author

@flodolo based on the discussion in the bug, Legacy won't be used and only Recommended will be added as a new string.

@flodolo
Copy link
Contributor

flodolo commented Feb 13, 2018

@flodolo based on the discussion in the bug, Legacy won't be used and only Recommended will be added as a new string.

Thanks, extracting that string then.

@pascalchevrel
Copy link
Member

pascalchevrel commented Feb 22, 2018

@flodolo are we good now with l10n coverage?

@flodolo
Copy link
Contributor

flodolo commented Feb 22, 2018

Yes, I think we're good (also, cc @peiying2)

Copy link
Contributor

@jpetto jpetto left a comment

Choose a reason for hiding this comment

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

Functionally looks great!

Would just like a couple changes for future code readability/comprehension.

@@ -341,7 +353,7 @@ class FirefoxAndroid(_ProductDetails):
'x86': archive_url_base + '-%s/fennec-%s.multi.android-i386.apk',
}

def platforms(self, channel='release'):
def platforms(self, channel='release', classified=False):
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add a comment here explaining why this method has an unused classified parameter?

@@ -265,7 +265,8 @@ def all_downloads(request, platform, channel):

context = {
'platform': platform,
'platforms': product.platforms(channel),
'platforms': product.platforms(channel, True),
'platform_cls': product.platform_classification,
Copy link
Contributor

Choose a reason for hiding this comment

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

While this works for FirefoxAndroid (I believe through the __getattr__ implementation in ProductDetails), I think it would be clearer to explicitly set plaform_classification = None (with a comment) in the FirefoxAndroid class in firefox_details.py.

This will make understanding/reading the code quite a bit simpler for those unfamiliar with ProductDetails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @jpetto! Added comments as well as plaform_classification = None for FirefoxAndroid.

…it version (to limit the number of 32 downloads)
@jpetto
Copy link
Contributor

jpetto commented Feb 28, 2018

Thanks @kyoshino!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants