Closed Bug 1023950 Opened 5 years ago Closed 5 years ago

Update visuals to latest spec for app download/cancel/error states

Categories

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

x86
macOS
defect
Not set

Tracking

(feature-b2g:2.0, b2g-v2.0 fixed, b2g-v2.1 fixed)

RESOLVED FIXED
2.0 S4 (20june)
feature-b2g 2.0
Tracking Status
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: jlal, Assigned: jlal)

References

Details

(Keywords: late-l10n, Whiteboard: [systemsfe])

Attachments

(13 files, 2 obsolete files)

596.36 KB, application/pdf
Details
518.80 KB, image/png
Details
298.33 KB, image/png
amylee
: ui-review-
Details
1.38 MB, image/png
Details
46 bytes, text/x-github-pull-request
kgrandon
: review+
kgrandon
: feedback+
crdlc
: feedback+
Details | Review
282.15 KB, image/png
Details
282.15 KB, image/png
Details
364.66 KB, image/png
Details
365.95 KB, image/png
pla
: review+
Details
367.68 KB, image/png
Details
151.41 KB, application/zip
Details
46 bytes, text/x-github-pull-request
Details | Review
46 bytes, text/x-github-pull-request
Details | Review
We landed most (if not all) of the work to bring this functionality to vertical home but we need to update it to the latest specs (attached)
QA Whiteboard: [VH-FL-blocking+][VH-FC-blocking+]
blocking-b2g: --- → 2.0?
I thought about it twice and as the feature is working, having new icons is not blocking 2.0, to my understanding.
blocking-b2g: 2.0? → ---
QA Whiteboard: [VH-FL-blocking+][VH-FC-blocking+] → [VH-FL-blocking-][VH-FC-blocking-]
Attached file AppInstallVisualAssets.zip (obsolete) —
Attached are the visual assets for app install.
Johen, please think again.  By not blocking, you are contributing to a poor product, and that is putting it mildly.  We can't keep considering these things as 'small'.  It's NOT polish.  It's the actual icons that are needed to communicate app install states!
blocking-b2g: --- → 2.0?
(In reply to Peter La from comment #3)
> Johen, please think again.  By not blocking, you are contributing to a poor
> product, and that is putting it mildly.  We can't keep considering these
> things as 'small'.  It's NOT polish.  It's the actual icons that are needed
> to communicate app install states!

Let me understand something - is the existing implementation right now, what is the current UX & what are the problems with it?
(In reply to Jason Smith [:jsmith] from comment #4)
> (In reply to Peter La from comment #3)
> > Johen, please think again.  By not blocking, you are contributing to a poor
> > product, and that is putting it mildly.  We can't keep considering these
> > things as 'small'.  It's NOT polish.  It's the actual icons that are needed
> > to communicate app install states!
> 
> Let me understand something - is the existing implementation right now, what
> is the current UX & what are the problems with it?

Fix wording - "What is the existing implementation right now & what is the current UX & what are the problems with it?"
Whiteboard: [systemsfe]
Assignee: nobody → jlal
This version of the patch only contains the "installing" state changes
Attachment #8439511 - Flags: ui-review?
This is super nice to have, and if we get to it we will certainly ask for approval. Being that homescreen1 does not have these nice states, it's considered feature work that we don't block on.

That said, I do hope we fix this and uplift it by next week. Moving to block vhomescreen.next for now.
Blocks: vertical-home-next
No longer blocks: vertical-homescreen
blocking-b2g: 2.0? → ---
I am still planning to work on this but after I fix some of our blocking bugs today expect the patch Monday (and all states should work in search/collection)
blocking-b2g: --- → 2.0?
feature-b2g: --- → -
Amy and Peter, we're trying to make a blocking decision here. Just to clarify the rationale, the new icon assets are required for *both* Visual Refresh (which should be sufficient to block on, as these are the assets partners are expecting) and vertical homescreen. The new vertical homescreen needs the larger icons in order to do the three column display AND four column display.

If either of you can post some side-by-side screenshots of how the old assets would perform in vertical homescreen, that would help, but the new assets *are* part of the Visual Refresh.
Flags: needinfo?(pla)
Flags: needinfo?(amlee)
blocking-b2g: 2.0? → 2.0+
Attached image InstallApp_Screens.png
Hi, 

Here is a side by side comparison of the spec icons and the current icons. As you can see, the current icons do not convey Installation Stopped or Download Failed and the retry download looks especially bad. I would say this is a blocker.
Flags: needinfo?(amlee)
Target Milestone: --- → 2.0 S4 (20june)
(In reply to Amy Lee [:amylee] from comment #12)
> Created attachment 8441365 [details]
> InstallApp_Screens.png
> 
> Hi, 
> 
> Here is a side by side comparison of the spec icons and the current icons.
> As you can see, the current icons do not convey Installation Stopped or
> Download Failed and the retry download looks especially bad. I would say
> this is a blocker.

Hi Amy,

Should you change the ui-review to -?
Comment on attachment 8439511 [details]
Installing an app (visuals)

Will need to see the rest of the states before giving this a +. Thanks
Attachment #8439511 - Flags: ui-review? → ui-review-
With more info from VD, I'm okay with the need to block on this, but I'd prefer to track this with the feature-b2g flag, as I'd call this feature work, rather than a bug.

Candice - Can you set the feature-b2g flag here?
Blocks: vertical-homescreen
No longer blocks: vertical-home-next
blocking-b2g: 2.0+ → ---
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking-] → [VH-FL-blocking+][VH-FC-blocking-]
Flags: needinfo?(cserran)
QA Whiteboard: [VH-FL-blocking+][VH-FC-blocking-] → [VH-FL-blocking+][VH-FC-blocking+]
I'm flagging this for Candice since I was just on IRC w/ her, know she has gone to sleep in Paris, and know she is totally fine with me doing this. :)
feature-b2g: - → 2.0
Flags: needinfo?(cserran)
Worth noting that one UX bug my patch will have right off the bat is if you restart your device and you have an app that was in an "Error" state it will be in a "Canceled" state when you see the homescreen again. This is because mozApps does not keep track of previous downloadErrors (there might be a work around)
Current version of my patch relies on a gecko fix for how events are sent through in Webapps.jsm
Depends on: 1027458
Attachment #8439508 - Attachment is obsolete: true
Attachment #8442635 - Flags: feedback?(kgrandon)
Comment on attachment 8442635 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20725

Could I get some early feedback on this patch? This is working well for me on the homescreen with my gecko patch but does not yet update the search or collections apps.
Attachment #8442635 - Flags: feedback?(crdlc)
Comment on attachment 8442635 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20725

LGTM, some comment on github but for me it is ok. Take care with my last change for loading asset in master. Thanks
Attachment #8442635 - Flags: feedback?(crdlc) → feedback+
Comment on attachment 8442635 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20725

I will look through more, I can be stubborn about WC, but we can change that in a follow-up. It shouldn't block this from landing.

For now I would probably prefer gaia_confirm/js/dialog.js to be moved outside of the component if possible, and into shared/js/homescreens or just shared/js possibly.
Attachment #8442635 - Flags: feedback?(kgrandon) → feedback+
Hey guys,

Sorry for the delayed response.  Basically, I agree with Amy's assessment, and I think everyone here does.  James. I'll take a look at your patch asap.
Flags: needinfo?(pla)
Comment on attachment 8442635 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20725

This is ready for a real review pass _but_ it will not handle retries correctly until the gecko patch lands (same bugs as we had before).
Attachment #8442635 - Flags: review?(kgrandon)
Attached image 2014-06-20-13-46-13.png
Attachment #8443603 - Flags: ui-review?(pla)
Attachment #8443604 - Flags: ui-review?(pla)
Attachment #8443605 - Flags: review?(pla)
Getting to each and very of these states can be difficult I wrote an interactive (command line interface) tool built on top of our testing framework  to test these manually ..
Attachment #8443608 - Flags: ui-review?(pla)
Comment on attachment 8443603 [details]
Failed install from search

Looks good.  The icon is not the right size but that is a separate issue.
Attachment #8443603 - Flags: ui-review?(pla) → ui-review+
Comment on attachment 8443604 [details]
Download fail on verticalhome

Looks good.
Attachment #8443604 - Flags: ui-review?(pla) → ui-review+
Comment on attachment 8443608 [details]
Download in progress on vertical home

Looks good.
Attachment #8443608 - Flags: ui-review?(pla) → ui-review+
Comment on attachment 8443605 [details]
Paused download on vertical home

Looks good.
Attachment #8443605 - Flags: review?(pla) → review+
This file contains the updated visuals, including a fix to the @3.375x size, which was mistakenly saved out at 189x189 instead of 284x284.
Attachment #8438824 - Attachment is obsolete: true
Assets updated and the failing test is ignored until we land 1027458 and it hits nightlies
Comment on attachment 8442635 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20725

Continuing to test, but everything is looking good.
Attachment #8442635 - Flags: review?(kgrandon) → review+
Keywords: late-l10n
Comment on attachment 8442635 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20725

Pre-emptive approval request for 2.0 as this will land today once the tree is green and is late-l10n.
Attachment #8442635 - Flags: approval-gaia-v2.0?(bbajaj)
Attachment #8442635 - Flags: approval-gaia-v2.0?(bbajaj) → approval-gaia-v2.0+
Comment on attachment 8442635 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20725

I made some comments on the bug. Many are nits / inconsistencies with the guidelines / or misused or bind for handleEvent - but the most important is that I really think we should not land the APZ workaround, and we need to open a bug for the comment about the visibilitychange event.

The APZ bug is likely hitting us in other apps, and if I understand it correctly it means that it is a regression (not because of the vertical homescreen) and should be fixed for 2.0, as it affect this app, and third party apps as well.
In master: https://github.com/mozilla-b2g/gaia/commit/f00f57bd824149a0bc9dede0af530fd712d15b3e
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.