987237 - Add Setting for cards view preview behavior

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Sam Foster [:sfoster] (he/him)]

Follow up from bug 972216, where we added the option to show the app icon or the screenshot in the cards view preview - for better performance on lower end devices. This behaviour should be toggleable at runtime via a Setting - and testable.

Comment 1

[Sam Foster [:sfoster] (he/him)]

Fixed unit test failure - moved early return in onviewport to allow the rotation class to get set when using the app icon vs. screenshot in card views. Carrying r+ from alive

Comment 2

[Sam Foster [:sfoster] (he/him)]

(In reply to Sam Foster [:sfoster] from comment #1)
> Fixed unit test failure - moved early return in onviewport to allow the
> rotation class to get set when using the app icon vs. screenshot in card
> views. Carrying r+ from alive

ack. Wrong bug. nm.

Comment 3

[Sam Foster [:sfoster] (he/him)]

Attachment: Add option to display appIcon in cards view

I think I got all the details we discussed. I'd like your feedback before I submit for review though. We didn't have any related category for this settings in the developer settings page, so I kinda tacked it on the end? Also if we nominate for uplift to 1.4, does the new string create a problem? 

Also, not sure if I'm doing the right thing about the localizable string that new setting requires. 
The tests pass for me, do they look ok and like they are testing what you would expect?

Comment 4

[Aus Lacroix [:aus]]

Comment on attachment 8401058 [details] [review]
Add option to display appIcon in cards view

(In reply to Sam Foster [:sfoster] from comment #3)
> Created attachment 8401058 [details] [review]
> Add option to display appIcon in cards view
> 
> I think I got all the details we discussed. I'd like your feedback before I
> submit for review though. We didn't have any related category for this
> settings in the developer settings page, so I kinda tacked it on the end?

That should be fine. :)

> Also if we nominate for uplift to 1.4, does the new string create a problem? 

We won't request uplift to 1.4 for this so no worries there. If it were to require uplift, I think we would not hold up the release for a missing developer setting string.

> Also, not sure if I'm doing the right thing about the localizable string
> that new setting requires.

Yep, you're doing it right. :)

> The tests pass for me, do they look ok and like they are testing what you
> would expect?

It looks like there are merge conflicts in cards_view.js. :\

For the most part this looks good! Just have to fix those conflicts and get it to a green state. :)

Comment 5

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8401058 [details] [review]
Add option to display appIcon in cards view

Fixed up the merge problem, green on travis.

Comment 6

[Sam Foster [:sfoster] (he/him)]

Attachment: Updated pull request to use SettingsListener

I messed something up and couldn't update the existing pull request. This updated patch just uses SettingsListener per :alive's request, and updates the tests for this change. Carrying r+.

Comment 7

[Michael Henretty [:mikehenrty][:mhenretty]]

master: https://github.com/mozilla-b2g/gaia/commit/bcda1083d494f3d94652c7b0ad4cf1babd4d0d1f