1180666 - Manage Homescreens

Status: RESOLVED FIXED

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

Comment 1

[Jacqueline Savory [:jsavory] UX]

I’ve attached a link to the initial IxD spec for replaceable homescreens in the user story section of the bug as well as here:

https://drive.google.com/open?id=0B8kj4Mlm-HJeVThwQ0hxWEo5SlU

This is just an initial draft, so there is more work to do on some of the stories, but feel free to provide any feedback on what is presented so far. Thanks!

Comment 2

[Chris Lord [:cwiiis]]

Looks nice! My only question is the columns layout part - that's really only specifically for the built-in home-screen, and we probably shouldn't encourage other apps to read its settings database.

What should we do about that? Should we allow homescreens some kind of configuration options in the settings? (and if so, how would we specify that, I guess in a similar way to whatever keyboards do?) And if we shouldn't, I guess we should move the configuration directly into the homescreen (perhaps via pinch gesture, as Kevin originally had it?)

Comment 3

[GitHub Autolander Bot]

Attachment: [gaia] gmarty:Bug-1180666-Manage-Homescreens > mozilla-b2g:master

Comment 4

[Guillaume Marty [:gmarty]]

Let's make this patch about update the Home Screen section of the Settings app.

Comment 5

[Guillaume Marty [:gmarty]]

Comment on attachment 8644934 [details] [review]
[gaia] gmarty:Bug-1180666-Manage-Homescreens > mozilla-b2g:master

Hi Fred. Can you review this patch please. It's pretty big but it implements the new specs from :jsavory with a few minor changes for consistency.
Feel free to ping me if needed.

Comment 6

[[e-mail]]

Comment on attachment 8644934 [details] [review]
[gaia] gmarty:Bug-1180666-Manage-Homescreens > mozilla-b2g:master

1. Seems vertical panel settings has been moved into #legacy-homescreen-layout in homescreen-details panel, it looks reasonable but is not matched with the UI spec attached above. Is there any updated UI spec available?

2. Besides that, I'd prefer keep main panel as `homescreens` panel (like `themes` panel) since new homescreen/s panel's responsibility is more like original homescreens panel.

3. the system update API is about to have a big change in bug 1161927, to make the migration smoother, please create a followup issue to deal with it and wait after bug 1161927 is landed.

Comment 7

[Guillaume Marty [:gmarty]]

Comment on attachment 8644934 [details] [review]
[gaia] gmarty:Bug-1180666-Manage-Homescreens > mozilla-b2g:master

Thanks Fred for your feedback.

1. Yes, you're right, the specs need updating. Jacqueline, can you change the specs to move the columns layout selection to the legacy vertical home screen detail page. This settings doesn't apply to the new home screen.

2. I rename the panel to homescreens and moved it under keyboard (I thought Languages and Keyboard section should be together).

3. The new update API looks good. I'll fill in a following bug to update and refactor the code.

Note:
Also, I renamed the 'homescreen' strings to 'home screen' which are correct.

Comment 8

[[e-mail]]

Attachment: split_radio.png

The radio button is truncated, the split line seems too far away from the button. 

Please also put a homescreen **with longer name** in the field and see how its going. (expect the name will change to next line and not overlap the radio)

Comment 9

[[e-mail]]

Attachment: legacy_home.png

Besides Icon layout, the vertical homescreen panel UI also not match the previous spec.

I'd review again once UI spec is revised.

Comment 10

[Guillaume Marty [:gmarty]]

I fixed the issues you left here and on Github.
Jacqueline is on PTO at the moment. I'm sure she'll update the specs as soon as she's back. Can you review the patch in the meantime? Thanks!

Comment 11

[[e-mail]]

Comment on attachment 8644934 [details] [review]
[gaia] gmarty:Bug-1180666-Manage-Homescreens > mozilla-b2g:master

can't open panel with following log:

E/Settings(16922): [JavaScript Error: "Error: Not loaded: panels/homescreens/wallpaper" {file: "app://settings.gaiamobile.org/js/vendor/alameda.js" line: 537}]

Comment 12

[Jacqueline Savory [:jsavory] UX]

Sorry about the delay on this one, just a couple comments:

1. I'm not sure about having the icon column layout in the homescreen details page. It feels a bit too buried, and I'm concerned that users will struggle to discover it. If possible, I think it would be best to leave the icon column option on the "Home Screens" page but disable it if a different homescreen is enabled and doesn't use this feature. 

2. We might want to leave the 'Home Screens' option in the Settings menu to just below 'Display'. The reason for this is that the main purpose of the 'Home Screen' page is now changing the wallpaper. If the option is too far down, users may see 'Display' and look for it there instead. 

There are a few more items that I am currently discussing with Guillaume, I'll update the spec once those have been reviewed. Also, Amy is also working on a visual spec that should answer a few of UI issues as well. Thanks!

Comment 13

[Guillaume Marty [:gmarty]]

Comment on attachment 8644934 [details] [review]
[gaia] gmarty:Bug-1180666-Manage-Homescreens > mozilla-b2g:master

Thanks Jacqueline for the feedback. I did the change you suggested here and the ones discussed offline with Amy.
I also did the code change from the previous code review, so I guess it's time for another r?.

Comment 14

[[e-mail]]

@gmarty thanks for update the patch. 
Since its a new feature with major UI change and Jacqueline is back, could you set ui review first before code review?

Comment 15

[Guillaume Marty [:gmarty]]

Comment on attachment 8644934 [details] [review]
[gaia] gmarty:Bug-1180666-Manage-Homescreens > mozilla-b2g:master

I took the discussion with UX offline. Amy sent me a round of changes that I implemented in the patch.
If everything looks fine, can we get a ui-review? Then the code will be ready for review. Thanks!

Comment 16

[Amy Lee [:amylee] UX]

Comment on attachment 8644934 [details] [review]
[gaia] gmarty:Bug-1180666-Manage-Homescreens > mozilla-b2g:master

Hi, 

I have a few edits that I'll attach to this bug. Thanks!

Comment 17

[Amy Lee [:amylee] UX]

Attachment: Homescreen_Edits_v1.pdf

UI review visual edits

Comment 18

[Chris Lord [:cwiiis]]

(In reply to Amy Lee [:amylee] UX from comment #17)
> Created attachment 8652962 [details]
> Homescreen_Edits_v1.pdf
> 
> UI review visual edits

In this edit, the app title would be listed twice - once in the header, then immediately below it. Is this what you intended?

Comment 19

[Amy Lee [:amylee] UX]

(In reply to Chris Lord [:cwiiis] from comment #18)
> (In reply to Amy Lee [:amylee] UX from comment #17)
> > Created attachment 8652962 [details]
> > Homescreen_Edits_v1.pdf
> > 
> > UI review visual edits
> 
> In this edit, the app title would be listed twice - once in the header, then
> immediately below it. Is this what you intended?

Hi, 

Yes. If the app name is truncated in the header, this allows the app name to be seen fully. 

Thanks

Comment 20

[GitHub Autolander Bot]

Attachment: [gaia] Cwiiis:Bug-1180666-Manage-Homescreens > mozilla-b2g:master

Comment 21

[Chris Lord [:cwiiis]]

Comment on attachment 8654214 [details] [review]
[gaia] Cwiiis:Bug-1180666-Manage-Homescreens > mozilla-b2g:master

I've made the suggested changes.

The spec has the column layout appearing on the main Home Screens panel, but we can't accomplish this readily because we need to read the homescreen setting to know whether to display it.

Seeing as the new homescreen will be the default and we likely won't have this setting here, I've chosen to leave it as Guillaume had it, in the specific homescreen's detail page.

Comment 22

[Amy Lee [:amylee] UX]

Attachment: Replaceable-Homescreen-Edits_v2.png

Visual edits

Comment 23

[Amy Lee [:amylee] UX]

Comment on attachment 8654214 [details] [review]
[gaia] Cwiiis:Bug-1180666-Manage-Homescreens > mozilla-b2g:master

Hi, 

I attached my edits to the bug. Thanks!

Comment 24

[Jacqueline Savory [:jsavory] UX]

Comment on attachment 8654214 [details] [review]
[gaia] Cwiiis:Bug-1180666-Manage-Homescreens > mozilla-b2g:master

Overall, this looks good to me. I think having the columns option in the details page is less discoverable but it still works. 

Just to confirm, Chris do you mean that the option to change between three and four columns will be removed completely from the default homescreen? 

Also, I'm guessing that when the Home Screens section is available in Marketplace, the "Get More Home Screens" button will take you directly to that screen? Currently, it displays three options but I'm assuming these will be removed?

Comment 25

[[e-mail]]

update about the system update API change in bug 1161927, 
<- it is not likely get change during 2.5, so you can put the update-check related code back for review.

Comment 26

[Chris Lord [:cwiiis]]

(In reply to Amy Lee [:amylee] UX from comment #22)
> Created attachment 8654314 [details]
> Replaceable-Homescreen-Edits_v2.png
> 
> Visual edits

re: the radio button, are you exporting the right pixel scale when you install gaia? The radio button displays correctly for me. I'll make the other changes mentioned.

Comment 27

[Chris Lord [:cwiiis]]

(In reply to Jacqueline Savory [:jsavory] UX from comment #24)
> Comment on attachment 8654214 [details] [review]
> [gaia] Cwiiis:Bug-1180666-Manage-Homescreens > mozilla-b2g:master
> 
> Overall, this looks good to me. I think having the columns option in the
> details page is less discoverable but it still works. 
> 
> Just to confirm, Chris do you mean that the option to change between three
> and four columns will be removed completely from the default homescreen? 

The option will be there, but how it will be exposed is still uncertain. The easiest thing, code-wise, is for the option to be contained in the homescreen (so either changed via pinching, or via the long-press menu) - however, we may need to introduce some way for homescreens to add settings to the homescreen settings menu.

> Also, I'm guessing that when the Home Screens section is available in
> Marketplace, the "Get More Home Screens" button will take you directly to
> that screen? Currently, it displays three options but I'm assuming these
> will be removed?

I think this is the case, Guillaume would know better (I'm just pushing this over the finish line while he's on PTO).

Comment 28

[Chris Lord [:cwiiis]]

(In reply to Fred Lin [:gasolin] from comment #25)
> update about the system update API change in bug 1161927, 
> <- it is not likely get change during 2.5, so you can put the update-check
> related code back for review.

I didn't work on this patch while this was being discussed, could you be more specific about the code you're referring to? What doesn't this patch do that it should at the moment?

Comment 29

[[e-mail]]

Base on comment 6. There's a section in PR that is commented out right now, which is referred to check if there's any new homescreen updated.

I don't know why we need that special update checking for homescreen, since it will be already part of marketplace update notice. @Jacqueline can you help elaborate that design?

Comment 31

[Chris Lord [:cwiiis]]

Attachment: Homescreens UI

I've tried to take into account all edit requests. There's a lot of shared CSS in Settings, so it makes it a little tricky, but I think, give a pixel or three, that most has been accounted for.

Comment 32

[Chris Lord [:cwiiis]]

Comment on attachment 8654214 [details] [review]
[gaia] Cwiiis:Bug-1180666-Manage-Homescreens > mozilla-b2g:master

Can I get a review on this, with the assumption that once jsavory's n? comes back, it will be that the update-check won't be necessary and I can remove the commented out code?

If we do need the update check, I'll re-flag for review, but as it is, this bug is really hampering the experience of replaceable homescreens + new-homescreen and it would be good to get it merged, given how long it's basically been finished.

We can always file follow-up bugs if there are small, remaining issues.

Comment 33

[Amy Lee [:amylee] UX]

Comment on attachment 8655988 [details]
Homescreens UI

Hi Chris, 

This is looking good. Just have one minor comment about the hairline having equal distance between top and bottom elements. I'll attach my edits.

Comment 34

[Amy Lee [:amylee] UX]

Attachment: homescreens-edits-v2.png

VSD edits

Comment 35

[Chris Lord [:cwiiis]]

(In reply to Amy Lee [:amylee] UX from comment #34)
> Created attachment 8656774 [details]
> homescreens-edits-v2.png
> 
> VSD edits

Is this the last issue? I'd really rather address small changes like this in follow-up, rather than go back and forth. This patch has been basically finished for several weeks and is being brought up a lot by QA. I don't think something being a few pixels off should stop us from committing this and refining it once it's in-tree, it's a lot more expensive in terms of developer and QA time to do it like this.

(the new homescreen icon being blurry is due to some missing icons in that app - there haven't been any icons provided for it yet, but that's not something we should address here)

Comment 36

[Amy Lee [:amylee] UX]

(In reply to Chris Lord [:cwiiis] from comment #35)
> (In reply to Amy Lee [:amylee] UX from comment #34)
> > Created attachment 8656774 [details]
> > homescreens-edits-v2.png
> > 
> > VSD edits
> 
> Is this the last issue? I'd really rather address small changes like this in
> follow-up, rather than go back and forth. This patch has been basically
> finished for several weeks and is being brought up a lot by QA. I don't
> think something being a few pixels off should stop us from committing this
> and refining it once it's in-tree, it's a lot more expensive in terms of
> developer and QA time to do it like this.
> 
> (the new homescreen icon being blurry is due to some missing icons in that
> app - there haven't been any icons provided for it yet, but that's not
> something we should address here)

Hi Chris, 

Yeah this is basically the last issue from me. I know it's pretty minor so if it makes life easier to have this as a separate bug then I'm okay with creating a follow-up bug.

Comment 37

[[e-mail]]

Comment on attachment 8654214 [details] [review]
[gaia] Cwiiis:Bug-1180666-Manage-Homescreens > mozilla-b2g:master

Thanks for trying push it forward.

But I saw `Not loaded: panels/homescreens/wallpaper` Error when I apply the patch and test it on device

```
E/Settings( 2257): [JavaScript Error: "Error: Not loaded: panels/homescreens/wallpaper" {file: "app://settings.gaiamobile.org/js/vendor/alameda.js" line: 537}]
```

And several unit test and gij failed in treeherder, please fix them and set review again.

Comment 38

[Jacqueline Savory [:jsavory] UX]

Hi Fred, 

To elaborate on the check for updates design, the button is there to check for all updates including homescreens. It is not intended to be there just for homescreens as they are integrated into the general updates flow. 

I've included it here to help users discover the update process for homescreens as I was worried they may not find it in the general updates section. Later, I think we can look into making this button specific for homescreens, but currently that isn't part of the update flow.

Comment 40

[GitHub Autolander Bot]

Attachment: [gaia] gmarty:Bug-1180666-Manage-Homescreens-2 > mozilla-b2g:master

Comment 41

[Guillaume Marty [:gmarty]]

Comment on attachment 8657807 [details] [review]
[gaia] gmarty:Bug-1180666-Manage-Homescreens-2 > mozilla-b2g:master

Thanks Chris for the work when I was away.
I believe the patch is now ready to review. I fixed the tests and added back the check update feature.
As agreed with Amy, we'll fix the visual issue in a following bug.

Comment 42

[[e-mail]]

Comment on attachment 8657807 [details] [review]
[gaia] gmarty:Bug-1180666-Manage-Homescreens-2 > mozilla-b2g:master

Thanks for update the patch, I think we are close to the goal.

Please also address comments left in https://github.com/mozilla-b2g/gaia/pull/31585


And please confirm if we still need `check app update` function here.

Comment 43

[Jacqueline Savory [:jsavory] UX]

Hi Fred and Guillaume, 

I've been getting some feedback about the 'check app update' function here and had a discussion with the UX team about whether we should keep it. We agreed that this is creating more confusion then it is helping and it would be best to remove it for now until we can improve the add-on/homescreen update flow. I've updated the spec to reflect this change. 

Sorry about the back and forth on this one!

Comment 44

[Amy Lee [:amylee] UX]

Link to VSD spec:
https://drive.google.com/a/mozilla.com/file/d/0B0tdos8pQ1T7TXlEM0hNSTY1YlU/view?usp=sharing

Comment 45

[Guillaume Marty [:gmarty]]

Comment on attachment 8657807 [details] [review]
[gaia] gmarty:Bug-1180666-Manage-Homescreens-2 > mozilla-b2g:master

I removed the check update section and addressed most of the comments left on Github for this patch and the one by Chris, but:
* I can't switch to gaia-switch because what is needed is gaia-radio and it hasn't been updated for +1 year, so it seems risky to use it now.
* I haven't done the syntactic changes suggested (like replacing fat arrows by function). They don't have any impact on code stability or performance and will just delay the landing of this patch for no added value. If they are absolutely critical, we will address them in a following bug as we agreed with UI.

Hope it's going to be ok this time.

Carrying the ui-r+ by jsavory from the previous patch.

Comment 46

[[e-mail]]

Comment on attachment 8657807 [details] [review]
[gaia] gmarty:Bug-1180666-Manage-Homescreens-2 > mozilla-b2g:master

Sorry to confuse you. Instead of gaia-components, We use gaia-switch & gaia-radio inside of gaia/shared/elements https://github.com/mozilla-b2g/gaia/tree/master/shared/elements/gaia_radio

which is mostly a wrapper for building block style. And we've migrated all pack-radio to gaia-radio for a month, so there's no reason to put old building block pack-radio related hack back.

Comment 47

[Guillaume Marty [:gmarty]]

I tried to use gaia-radio but it didn't work so well.
In the home screens list, the label should be clickable and lead to the home screen details, while the radio button should change the home screen.
This UI pattern is also used in the USB storage and it still uses pack-split. Do you have any plan to replace it by gaia-radio? If no, my patch is good enough for now.

Comment 48

[[e-mail]]

The USB panel is redesigned in bug 1194045 and removed the split view.

The apn_list also use gaia-radio with split view in Cellular & Data > APN Settings > Data Settings panel.

https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/js/panels/apn_list/apn_template_factory.js

You could reference it instead of adding pack-radio back.

Comment 49

[Guillaume Marty [:gmarty]]

Comment on attachment 8657807 [details] [review]
[gaia] gmarty:Bug-1180666-Manage-Homescreens-2 > mozilla-b2g:master

Thanks for the reference usage of gaia-radio. That's what I needed.

I updated the patch to use it and removed the line related to pack-split. If there are any other non urgent changes, they'll be addressed in following bugs. Can I get a r+ now?

Comment 50

[[e-mail]]

Comment on attachment 8657807 [details] [review]
[gaia] gmarty:Bug-1180666-Manage-Homescreens-2 > mozilla-b2g:master

Looks fine to me with a couple of nits on github, please address them before merge. Thanks!

Comment 51

[[e-mail]]

Attachment: rtl splot issue

For RTL view the split and chevron seems not show correctly. Please confirm with UX and file related bugs to address that.

Comment 52

[Carsten Book [:Tomcat]]

https://github.com/mozilla-b2g/gaia/commit/588ff218e25d806fba319b5e614aab2c07a7b512