Closed Bug 1165416 Opened 5 years ago Closed 5 years ago

Update Pocket code to latest version (May 15th code drop)

Categories

(Firefox :: Pocket, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 41
Tracking Status
firefox38.0.5 --- verified
firefox39 --- fixed
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: nate, Assigned: nate)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch may15Drop.patch (obsolete) — Splinter Review
1) Removes panel AB test (still has learn more link test)
2) Removes FF logo from "Sign-up with Firefox" button when in the menu panel (to fix overlapping text issues with longer translations)

Note, we still need to show the "hero" version of the panel in the menu panel because it fits better in the narrow view. We'll target updating that in FF 39 so we can remove the additional images/code to support it.
Attachment #8606411 - Flags: review?(jaws)
Attachment #8606411 - Flags: review?(dolske)
Comment on attachment 8606411 [details] [diff] [review]
may15Drop.patch

Review of attachment 8606411 [details] [diff] [review]:
-----------------------------------------------------------------

r=me if you change setting.match to setting.contains.

::: browser/components/pocket/pktApi.js
@@ +604,5 @@
>       * Helper function to get current signup AB group the user is in
>       */
>      function getSignupAB() {
> +    	var setting = getSetting('signupAB');
> +        if (!setting || setting.match('hero'))

String.match expects a regular expression. 'hero' is a valid regular expression, but is overkill for this. We can just use `setting.contains('hero')`.
Attachment #8606411 - Flags: review?(jaws) → review+
Changing match to contains per :jaws review.
Attachment #8606411 - Attachment is obsolete: true
Attachment #8606411 - Flags: review?(dolske)
Attachment #8606459 - Flags: review?(jaws)
Attachment #8606459 - Flags: review?(dolske)
Attachment #8606459 - Flags: review?(dolske) → review+
Comment on attachment 8606459 [details] [diff] [review]
May15dropv2.patch

Approval Request Comment
[Feature/regressing bug #]: pocket
[User impact if declined]: text overlapping with icon on pocket panel when in firefox menu, and usage of an AB test that is no longer needed
[Describe test coverage new/current, TreeHerder]: manual testing
[Risks and why]: not much risk expected, changes limited to Pocket
[String/UUID change made/needed]: none
Attachment #8606459 - Flags: approval-mozilla-release?
Attachment #8606459 - Flags: approval-mozilla-beta?
Attachment #8606459 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/ebc545ec200a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Comment on attachment 8606459 [details] [diff] [review]
May15dropv2.patch

Should be in 38.0.5beta3.
Attachment #8606459 - Flags: approval-mozilla-release?
Attachment #8606459 - Flags: approval-mozilla-release+
Attachment #8606459 - Flags: approval-mozilla-beta?
Attachment #8606459 - Flags: approval-mozilla-beta+
Attachment #8606459 - Flags: approval-mozilla-aurora?
Attachment #8606459 - Flags: approval-mozilla-aurora+
Setting qe-verify+ for the second item from Comment 0.
Flags: qe-verify+
QA Contact: andrei.vaida
(In reply to Nate Weiner from comment #0)
> 2) Removes FF logo from "Sign-up with Firefox" button when in the menu panel
> (to fix overlapping text issues with longer translations)

I'm still seeing the Firefox logo next to the "Sign-up with Firefox" button label on 38.0.5b3 (20150518141916): http://i.imgur.com/MlaLDiN.png. Nate/Jared, have I misinterpreted Comment 0?

Tested with Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.
Blocks: Pocket
Flags: needinfo?(jaws)
Hi Andrei,

The Firefox logo should only be hidden on German, Japanese, Spanish, and Russian locales. It will still be present on en-US.
Flags: needinfo?(jaws)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12)
> Hi Andrei,
> 
> The Firefox logo should only be hidden on German, Japanese, Spanish, and
> Russian locales. It will still be present on en-US.

I've checked these locales on RC 38.0.5-build4 (20150525141253), using Windows 7 (x64) and the logo _is visible_ there as well:
* German locale (de): http://i.imgur.com/IT649FG.png
* Japanese locale (ja): http://i.imgur.com/W43N8wG.png
* Spanish locale (es-ES): http://i.imgur.com/ibas0fv.png
* Russian locale (ru): http://i.imgur.com/jPRjdwf.png
Hi Andrei, it should only be hidden when the Pocket button is located in the Firefox menu (not on the toolbar). Sorry for the confusion.
Thanks for the additional clarifications, Jared. Closing this, based on the test results from RC 38.0.5-build4 (20150525141253). Follow-up issues, if any, will be filed separately.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.