1147092 - [Add-On Manager] Visual refinements for list-view

Status: RESOLVED FIXED

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

Description

[Amy Lee [:amylee] UX]

This bug is for visual refinements for list-view in the add-on manager.

Comment 1

[Amy Lee [:amylee] UX]

Attachment: Manage_Addons_Listview_Edits.png

Hi, 

Here are my visual edits for the app manager (see attached for visual reference): 

1. Add Icon – “+” icon should be #858585

2. Sub headers are missing - “MY ADD-ONS” and “DOWNLOADED ADD-ONS”

3. App Icons – Remove drop shadow on icon. Icon should be sized at 50px x 50px.

4. Default app icon needs to be updated. Will provide graphic. 

5. App names text – Font colour should be #4d4d4d

6. Enable/Disabled Text – Colour should be #858585

Please refer to the web components for styling: 
http://gaia-components.github.io/gaia-components/

And also Hackerplace app in Lightsaber build (copy sub header style).

Thanks!

Comment 2

[Doug Sherk (:drs) (inactive)]

Arthur, as we discussed offline, can you take this?

Note that 1. looks like an edit that only Lightsaber will need.

I think 2. is bogus as we have no way to distinguish those categories. Amy, could you explain what the difference is between "my" and "downloaded" add-ons?

Yura, would you make the change for 1. on the lightsaber branch? Would be best to file a follow-up bug with status-b2g-v3.0:unaffected, I think.

Comment 3

[Arthur Chen [:arthurcc][inactive after 6/30]]

Amy, I noticed that the style of the reference mock is not consistent with other panels of settings app, is this intended?

Comment 4

[Amy Lee [:amylee] UX]

(In reply to Doug Sherk (:drs) (use needinfo?) from comment #2)
> Arthur, as we discussed offline, can you take this?
> 
> Note that 1. looks like an edit that only Lightsaber will need.
> 
> I think 2. is bogus as we have no way to distinguish those categories. Amy,
> could you explain what the difference is between "my" and "downloaded"
> add-ons?
> 
> Yura, would you make the change for 1. on the lightsaber branch? Would be
> best to file a follow-up bug with status-b2g-v3.0:unaffected, I think.

"My Add-Ons" refers to the add-ons that the user created with their device either through the customizer or theme editor. 

"Downloaded Add-Ons" refers to add-ons from another source (i.e Hackerplace or transferred from P2P Sharing)

Comment 5

[Amy Lee [:amylee] UX]

(In reply to Arthur Chen [:arthurcc] from comment #3)
> Amy, I noticed that the style of the reference mock is not consistent with
> other panels of settings app, is this intended?

Hi Arthur, 

Yes, this is intentional. We are using the styling from web components. The current Settings style is outdated. Thanks

Comment 6

[Doug Sherk (:drs) (inactive)]

(In reply to Amy Lee [:amylee] from comment #4)
> "My Add-Ons" refers to the add-ons that the user created with their device
> either through the customizer or theme editor. 
> 
> "Downloaded Add-Ons" refers to add-ons from another source (i.e Hackerplace
> or transferred from P2P Sharing)

Ok, I think we actually can do that by adding an arbitrary flag to the manifest of Customizer-generated add-ons when we create them.

However, this will be on Lightsaber only. The master branch and Settings app have no concept of a Customizer.

Themes don't show up in the Add-on Manager. There's a "Themes" panel that we will have to modify to get our desired result, but that's a whole other issue.

Needinfoing Justin for visibility on the add-on generation side. Yura, the Settings side of this is another part you'll have to take.

Comment 7

[Amy Lee [:amylee] UX]

Attachment: Icon_Default.zip

Updated default app icon to replace current one in Add-On manager.

Comment 8

[Arthur Chen [:arthurcc][inactive after 6/30]]

Currently there is no plan switching to the new style (that may also include the style of the default icon), to keep the consistency I would suggest to stick with what we have. Does that make sense to you?

Comment 9

[Amy Lee [:amylee] UX]

Attachment: Add-On_Screen_Edits.png

Hi, 

Here are the visual edits for the add-on description screen. Basically this screen looks identical to the add-on description screen in the Hackerplace App with the addition of an Enable Add-On toggle at the top and the "Share", "Delete" buttons at the bottom. I would pull the styling from the Hackerplace App:

Add-On Description Screen Edits
-------------------------------
1. Change to “Add-On and with Enabled/Disabled message underneath. 
Font colour: Dark grey #4d4d4d, Light grey: #858585

2. Remove Description header

3. Please reference mock and also Hackerplace App for styling of description section (font colour, font size and icon.) 

4. Please reference mock and also Hackerplace App for styling of “affected apps” section. 

5. Remove “Options” header. 

6. Please use button style in web components.


Thanks!

Comment 10

[Doug Sherk (:drs) (inactive)]

The Hackerplace app is here, for reference: https://github.com/fxos/directory/

Comment 11

[Amy Lee [:amylee] UX]

(In reply to Arthur Chen [:arthurcc] from comment #8)
> Currently there is no plan switching to the new style (that may also include
> the style of the default icon), to keep the consistency I would suggest to
> stick with what we have. Does that make sense to you?

Hi Arthur, 

Right now all the Lightsaber apps are styled with the web components. This was brought up within the design team early on if we should use the web components styles or stick with the old ones and it was agreed to use the new components. We want all the Lightsaber apps to have the same visual treatment. Hope that helps!

Comment 12

[Amy Lee [:amylee] UX]

(In reply to Amy Lee [:amylee] from comment #11)
> (In reply to Arthur Chen [:arthurcc] from comment #8)
> > Currently there is no plan switching to the new style (that may also include
> > the style of the default icon), to keep the consistency I would suggest to
> > stick with what we have. Does that make sense to you?
> 
> Hi Arthur, 
> 
> Right now all the Lightsaber apps are styled with the web components. This
> was brought up within the design team early on if we should use the web
> components styles or stick with the old ones and it was agreed to use the
> new components. We want all the Lightsaber apps to have the same visual
> treatment. Hope that helps!

Also an FYI that the default icon has been updated in the homescreen yesterday and we should be using the updated default icon that I attached. Thanks

Comment 13

[GitHub Autolander Bot]

Attachment: [gaia] yzen:bug-1147092 > mozilla-b2g:lightsaber

Comment 14

[Arthur Chen [:arthurcc][inactive after 6/30]]

(In reply to Amy Lee [:amylee] from comment #11)
> (In reply to Arthur Chen [:arthurcc] from comment #8)
> > Currently there is no plan switching to the new style (that may also include
> > the style of the default icon), to keep the consistency I would suggest to
> > stick with what we have. Does that make sense to you?
> 
> Hi Arthur, 
> 
> Right now all the Lightsaber apps are styled with the web components. This
> was brought up within the design team early on if we should use the web
> components styles or stick with the old ones and it was agreed to use the
> new components. We want all the Lightsaber apps to have the same visual
> treatment. Hope that helps!

Hi Amy,

As the add-on manger is part of the settings app so I incline to make the style consistent, so the changes that I am going to make in master would be updating the default icon and icon size, and adding the sub headers. The complete refinements will be lightsaber specific for this time being. Thanks!

Comment 15

[Amy Lee [:amylee] UX]

(In reply to Arthur Chen [:arthurcc] from comment #14)
> (In reply to Amy Lee [:amylee] from comment #11)
> > (In reply to Arthur Chen [:arthurcc] from comment #8)
> > > Currently there is no plan switching to the new style (that may also include
> > > the style of the default icon), to keep the consistency I would suggest to
> > > stick with what we have. Does that make sense to you?
> > 
> > Hi Arthur, 
> > 
> > Right now all the Lightsaber apps are styled with the web components. This
> > was brought up within the design team early on if we should use the web
> > components styles or stick with the old ones and it was agreed to use the
> > new components. We want all the Lightsaber apps to have the same visual
> > treatment. Hope that helps!
> 
> Hi Amy,
> 
> As the add-on manger is part of the settings app so I incline to make the
> style consistent, so the changes that I am going to make in master would be
> updating the default icon and icon size, and adding the sub headers. The
> complete refinements will be lightsaber specific for this time being. Thanks!

Hi Arthur, 

Just to clarify, that we are not making changes to the icon size, just the style. If the icon that I provided isn't the right size please let me know. Also the sub-headers should only be for lightsaber along with all visual edits. Thanks

Comment 16

[Yura Zenevich [:yzen] (please use "needinfo")]

Merge commit for 1. : https://github.com/mozilla-b2g/gaia/commit/b1282edeab9f1ae00aeee76816fe3ea30d56b8a0

Comment 17

[GitHub Autolander Bot]

Attachment: [gaia] crh0716:1147092 > mozilla-b2g:master

Comment 18

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8592082 [details] [review]
[gaia] crh0716:1147092 > mozilla-b2g:master

EJ, would you mind check this simple patch? I only made the change as the button is not used in any other panels so there will be no inconsistency. Thanks!

Comment 19

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8592082 [details] [review]
[gaia] crh0716:1147092 > mozilla-b2g:master

Cancel the review as there are visuals can be refined in the detail panel.

Comment 20

[Arthur Chen [:arthurcc][inactive after 6/30]]

Attachment: screenshot

Amy, I was trying do the refinement based on the existing style of settings app. Would you mind review the result? Thanks!

Comment 21

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8592082 [details] [review]
[gaia] crh0716:1147092 > mozilla-b2g:master

EJ, the patch is ready for review now. It composes two parts. The first part is for adding the sub title showing the current enabled state of the addon. The second part is for other refinements. Could you help review it? Thanks.

Comment 22

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

@Arthur,

there are some missing parts that need to be clarified (we should fix our CSS or the spec should be updated). In order to make things clear, I would list down the notes below :

[In addons section]

1. The app icon is not vertically aligned. (need to fix code)
2. The title of this section is not "Manage Add-ons" (need to fix code)
3. The app icon is not 5rem X 5rem (Right now, it is 3rem X 3rem) (need to fix spec)
4. Color for app name & app subtitle are different than what spec defined (need to fix spec)

And about codes, I only got two concerns here. Firstly, because we are going to override CSS in <gaia-header>, instead of using "!important", is there any other better way to do so ? I think at first when Wilson designed <gaia-header>, there must be some ways for us to customize the CSS (or the only way is yes, override it with "!important" XD?)

Secondly, I noticed that in addon_details/panel.js, if we set the `enabled` to true / false here when "onBeforeShow" is calling, because we can't control the animation time, so there would be a small lag when doing "panel transition" & "toggle transition". I think we can move this to "onShow", what do you think XD?

That's it ! For the other missing parts, leave them to ui-review ! Thanks !!

Comment 23

[Arthur Chen [:arthurcc][inactive after 6/30]]

(In reply to EJ Chen [:eragonj][:小龍哥][ni? if you need me] from comment #22)
> @Arthur,
> 
> there are some missing parts that need to be clarified (we should fix our
> CSS or the spec should be updated). In order to make things clear, I would
> list down the notes below :
> 
> [In addons section]
> 
> 1. The app icon is not vertically aligned. (need to fix code)
I will fix this.
> 2. The title of this section is not "Manage Add-ons" (need to fix code)
In general the text in the menu item and the header should be the same, so for this item we probably need to update the spec.
> 3. The app icon is not 5rem X 5rem (Right now, it is 3rem X 3rem) (need to
> fix spec)
The icon size is the same as the app permission panel.
> 4. Color for app name & app subtitle are different than what spec defined
> (need to fix spec)
The entire lightsaber spec was based on gaia component, and what we are trying to do here is simply fallback to the current style of settings app. Creating a spec using the current style may not be necessary IMO.
> 
> And about codes, I only got two concerns here. Firstly, because we are going
> to override CSS in <gaia-header>, instead of using "!important", is there
> any other better way to do so ? I think at first when Wilson designed
> <gaia-header>, there must be some ways for us to customize the CSS (or the
> only way is yes, override it with "!important" XD?)
As far as I know there is no straight forward way of doing this for this time being. There is a paragraph describing this in this thread[1].
> 
> Secondly, I noticed that in addon_details/panel.js, if we set the `enabled`
> to true / false here when "onBeforeShow" is calling, because we can't
> control the animation time, so there would be a small lag when doing "panel
> transition" & "toggle transition". I think we can move this to "onShow",
> what do you think XD?
Are you suggesting that the toggle may be still in transition when users see the panel? 
> 
> That's it ! For the other missing parts, leave them to ui-review ! Thanks !!

[1]: https://groups.google.com/forum/#!msg/mozilla.dev.gaia/PnWo7aoCS_A/Zp5zusdJg3oJ

Comment 24

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(In reply to Arthur Chen [:arthurcc] from comment #23)
> (In reply to EJ Chen [:eragonj][:小龍哥][ni? if you need me] from comment #22)
> > Secondly, I noticed that in addon_details/panel.js, if we set the `enabled`
> > to true / false here when "onBeforeShow" is calling, because we can't
> > control the animation time, so there would be a small lag when doing "panel
> > transition" & "toggle transition". I think we can move this to "onShow",
> > what do you think XD?
> Are you suggesting that the toggle may be still in transition when users see
> the panel? 

I did see that sometimes, not sure whether this would happen there in your device ?

Comment 25

[Arthur Chen [:arthurcc][inactive after 6/30]]

(In reply to EJ Chen [:eragonj][:小龍哥][ni? if you need me] from comment #24)
> (In reply to Arthur Chen [:arthurcc] from comment #23)
> > (In reply to EJ Chen [:eragonj][:小龍哥][ni? if you need me] from comment #22)
> > > Secondly, I noticed that in addon_details/panel.js, if we set the `enabled`
> > > to true / false here when "onBeforeShow" is calling, because we can't
> > > control the animation time, so there would be a small lag when doing "panel
> > > transition" & "toggle transition". I think we can move this to "onShow",
> > > what do you think XD?
> > Are you suggesting that the toggle may be still in transition when users see
> > the panel? 
> 
> I did see that sometimes, not sure whether this would happen there in your
> device ?

After the discussion we believe that it was inspector attached at that time. The transition should be as expected in normal cases.

Comment 26

[Arthur Chen [:arthurcc][inactive after 6/30]]

Hi Amy, any chance to help review the UI? Thanks.

Comment 27

[Stephany Wilkes]

Amy is on PTO. Changing flag to Eric.

Comment 28

[GitHub Autolander Bot]

Attachment: [gaia] yzen:bug-1147092 > mozilla-b2g:master

Comment 29

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8596738 [details] [review]
[gaia] yzen:bug-1147092 > mozilla-b2g:master

Same PR for + button but for master

Comment 30

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8596738 [details] [review]
[gaia] yzen:bug-1147092 > mozilla-b2g:master

Hi Yura, I've already included that part in my patch, so I just cancel the review.

Comment 31

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Arthur Chen [:arthurcc] from comment #30)
> Comment on attachment 8596738 [details] [review]
> [gaia] yzen:bug-1147092 > mozilla-b2g:master
> 
> Hi Yura, I've already included that part in my patch, so I just cancel the
> review.

Thanks, Arthur!

Comment 32

[Eric Pang [:epang] UX]

Comment on attachment 8592082 [details] [review]
[gaia] crh0716:1147092 > mozilla-b2g:master

R- Based on the remaining work on Amy's edits list.  Will attach an updated edits list shortly.

Comment 33

[Eric Pang [:epang] UX]

Attachment: Manage_Addons_Edits.png

Before Amy went on PTO she mentioned that the web components might be able to be used.

Reading through the bug it sounds like it was not possible to use them.  Arthur can you clarify?  Amy will return on Monday so she'll be able to provide a ui-review based on the exiting styling.

I've also attached an updated version of Amy's edit list.

Comment 34

[Arthur Chen [:arthurcc][inactive after 6/30]]

It is not possible to use the new style for this time being while other panels are still using the original one. This also applies to the icon size and the text colors.

Comment 35

[Amy Lee [:amylee] UX]

(In reply to Arthur Chen [:arthurcc] from comment #26)
> Hi Amy, any chance to help review the UI? Thanks.

Hi Arthur, 

Please send me an UI review after Eric's updated edits have been made.

I also noticed the following:

1. The old icon is still being used. There is an updated icon attached to this bug. 
2. The "rename" text button is missing in the app screen. Is this in a separate bug?
3. Is there any way to override the current style for icon size and text colours? The icon is just too small as is. 

Thanks

Comment 36

[Amy Lee [:amylee] UX]

Comment on attachment 8592705 [details]
screenshot

Eric had done a UI review while I was on PTO. Removing UI review.

Comment 37

[Arthur Chen [:arthurcc][inactive after 6/30]]

(In reply to Amy Lee [:amylee] from comment #35)
> (In reply to Arthur Chen [:arthurcc] from comment #26)
> > Hi Amy, any chance to help review the UI? Thanks.
> 
> Hi Arthur, 
> 
> Please send me an UI review after Eric's updated edits have been made.
> 
> I also noticed the following:
> 
> 1. The old icon is still being used. There is an updated icon attached to
> this bug. 
This will be fixed in the revised patch.
> 2. The "rename" text button is missing in the app screen. Is this in a
> separate bug?
The patch was just merged back to master. I'll include it in my patch.
> 3. Is there any way to override the current style for icon size and text
> colours? The icon is just too small as is.
After confirmed with Jenny we decided to make the icon size align to the size in app permission panel. 
> 
> Thanks

Comment 38

[Arthur Chen [:arthurcc][inactive after 6/30]]

Attachment: addon.jpg

Hi Amy,

Would you mind review the latest screenshot? Note that we are not moving to the web component style after confirmed with Jenny. The font size and style for menu items are kept. As for the sub header for downloaded addon, the feature is not implemented yet.

Thanks!

Comment 39

[Amy Lee [:amylee] UX]

(In reply to Arthur Chen [:arthurcc] from comment #38)
> Created attachment 8599812 [details]
> addon.jpg
> 
> Hi Amy,
> 
> Would you mind review the latest screenshot? Note that we are not moving to
> the web component style after confirmed with Jenny. The font size and style
> for menu items are kept. As for the sub header for downloaded addon, the
> feature is not implemented yet.
> 
> Thanks!

Hi Arthur, 

Can you send me the link to the patch as well? I'd like to view it on the device. Thanks!

Comment 40

[Arthur Chen [:arthurcc][inactive after 6/30]]

It is attachment 8596738 [details] [review], thanks.

Comment 41

[Arthur Chen [:arthurcc][inactive after 6/30]]

Sorry it should be attachment 8592082 [details] [review].

Comment 42

[Amy Lee [:amylee] UX]

(In reply to Arthur Chen [:arthurcc] from comment #41)
> Sorry it should be attachment 8592082 [details] [review]

Hi Arthur, 

I am having issues flashing the patch and also had IT take a look and he couldn't seem to figure it out. When I tried to flash the patch I get this error: 

Exception: L10nError: "pair-view-title" not found in en-US in app://settings.gaiamobile.org

[failed] building settings app failed with exit code 1
make: *** [app] Error 1

Comment 43

[Arthur Chen [:arthurcc][inactive after 6/30]]

(In reply to Amy Lee [:amylee] from comment #42)
> (In reply to Arthur Chen [:arthurcc] from comment #41)
> > Sorry it should be attachment 8592082 [details] [review]
> 
> Hi Arthur, 
> 
> I am having issues flashing the patch and also had IT take a look and he
> couldn't seem to figure it out. When I tried to flash the patch I get this
> error: 
> 
> Exception: L10nError: "pair-view-title" not found in en-US in
> app://settings.gaiamobile.org

It seems you are using an old version of master. Could you try to rebase to the latest master before applying the PR?

> 
> [failed] building settings app failed with exit code 1
> make: *** [app] Error 1

Comment 44

[Amy Lee [:amylee] UX]

Comment on attachment 8592082 [details] [review]
[gaia] crh0716:1147092 > mozilla-b2g:master

Hi Arthur, 

Here's my feedback:

1. Change font size for description/affected apps to 1.7rems, line height 2.3 rems to match the rest of settings screens (See attached).

2. Under Affect Apps please have the spacing distance above and below the text the same (See attached).

3. Spoke to Jenny about icon sizes in the Add-On list view and she's okay with increasing the size. Please make the icon size in Add-Ons 50x50px.

Comment 45

[Amy Lee [:amylee] UX]

Attachment: AddOn_Edits.png

Edits for add-on manager screen

Comment 46

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8592082 [details] [review]
[gaia] crh0716:1147092 > mozilla-b2g:master

Hi Amy, all comments were addressed, would you mind take a look at it again? Thanks.

Comment 47

[Amy Lee [:amylee] UX]

Comment on attachment 8592082 [details] [review]
[gaia] crh0716:1147092 > mozilla-b2g:master

Hi Arthur, 

Thanks for making the changes. Noticed 2 minor things. 

1. Text block should be vertically centered to icon (see attached)
2. Move icon 2px to the left to align with text (see attached) 

I'll + the review after these adjustments. Thanks.

Comment 48

[Amy Lee [:amylee] UX]

Attachment: Edits_v2.png

Visual Edits

Comment 49

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8592082 [details] [review]
[gaia] crh0716:1147092 > mozilla-b2g:master

Comment addressed, mind take a look at it again? Thanks!

Btw, should I also wait for the review from Jacqueline?

Comment 50

[Amy Lee [:amylee] UX]

Comment on attachment 8592082 [details] [review]
[gaia] crh0716:1147092 > mozilla-b2g:master

Looks good! Thanks Arthur

Comment 51

[Jacqueline Savory [:jsavory] UX]

Comment on attachment 8592082 [details] [review]
[gaia] crh0716:1147092 > mozilla-b2g:master

This looks good to me as well!

Comment 52

[Arthur Chen [:arthurcc][inactive after 6/30]]

Thanks all!!

Comment 53

[GitHub Autolander Bot]

Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.

Comment 54

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8592082 [details] [review]
[gaia] crh0716:1147092 > mozilla-b2g:master

Ah... I thought I've already got an r+.

EJ, could you help review the patch again as the visual part has bee finalized? Thanks!

Comment 55

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8592082 [details] [review]
[gaia] crh0716:1147092 > mozilla-b2g:master

Thanks Arthur, r+ for this patch !

Comment 56

[Arthur Chen [:arthurcc][inactive after 6/30]]

Thanks, EJ.

master: 3654ec1d7ce1e0a56a34d5c3b06f6a9b33ff79ad

Comment 57

[Justin D'Arcangelo [:justindarc]]

Clearing NI? as this has already been resolved.