814506 - [Browser] Start page bookmark star should be greyed out

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Browser, defect, P3)

Description

[Ian Barlow (:ibarlow)]

Since we can't bookmark the start page, the star should be greyed out here. Either that, or we should allow the start page to be bookmarked.

Comment 1

[Patryk Adamczyk [:patryk] UX]

Agreed.

Comment 2

[Ben Francis [:benfrancis]]

Patryk, do you want to provide an asset for the disabled state of the bookmark icon?

Comment 3

[Patryk Adamczyk [:patryk] UX]

Can you please use opacity of 50% for disabled action icons.

Comment 4

[Eric Pang [:epang] UX]

Hi Ben, will you be able to implement this for v1.2?

Comment 5

[[e-mail]]

take it

Comment 6

[[e-mail]]

Attachment: pull request redirect to github

add style in hide/showStartscreen to greyed out the bookmark-button

Comment 7

[Ben Francis [:benfrancis]]

Comment on attachment 755826 [details]
pull request redirect to github

Thanks for the patch Fred.

Can we use the disabled attribute instead of a "greyed" class to be consistent with the back and forward buttons, and have it set to disabled in the HTML by default so that it doesn't flash from enabled to disabled on startup?

Otherwise it seems to work well :)

Comment 8

[Ben Francis [:benfrancis]]

Oh also, if we're going to use 0.5 opacity for the bookmark button as Patryk says, I think we should probably do that for the back button as well and remove the alternative image file because at the moment I think they're different shades of grey which looks a bit odd.

Comment 9

[[e-mail]]

Attachment: startscreen, all button disabled with opacity 0.5

ni? patrick, 

Here is the screenshot of startscreen, all button disabled with opacity 0.5.

Are you fine with removing backbutton disabled image back-disabled.png/[e-mail] file and use opacity 0.5 instead?

Comment 10

[[e-mail]]

Hi ben,

Thanks for review. I've changed the PR and use 'disable' attribute to grey out bookmark button and back button to opacity 0.5.

Comment 11

[Patryk Adamczyk [:patryk] UX]

Spoke to Ian (ibarlow) and since both icons are disabled indefinitely on that page, I think we should just remove the toolbar altogether.

Comment 12

[[e-mail]]

Comment on attachment 755826 [details]
pull request redirect to github

pull back review since the UI spec has changed...

Comment 13

[[e-mail]]

move to higher priority issue

Comment 14

[Ben Francis [:benfrancis]]

Clearing NEEDINFO, don't think there's any open questions on me here, we just need to implement the UX change.

Comment 15

[Dale Harvey (:daleharvey)]

Attachment: Pointer to GH

Comment 16

[Ben Francis [:benfrancis]]

Comment on attachment 765700 [details]
Pointer to GH

See comments in pull request https://github.com/mozilla-b2g/gaia/pull/10518

Comment 17

[Dale Harvey (:daleharvey)]

Comment on attachment 765700 [details]
Pointer to GH

Updated to fix startup visuals

Comment 18

[Ben Francis [:benfrancis]]

Comment on attachment 765700 [details]
Pointer to GH

r+me

Comment 19

[Ben Francis [:benfrancis]]

Merged into master in https://github.com/mozilla-b2g/gaia/commit/c25e3520dda20f158980236b9f1392c0b83de992

Comment 20

[isabelrios]

Commenting as this bug is in the list Resol-fixed leorun4 bugs to be tested. 
The start is not greyed but it is not enabled. 
Checked with latest leo build and unagi device v1-train 07/17 build:
Gecko-2d17cfb
Gaia-773a197

Would that be ok as the expected behaviour? Adding ni Ian Barlow

Comment 21

[Ian Barlow (:ibarlow)]

(In reply to Isabel Rios [:isabel_rios] from comment #20)
> Commenting as this bug is in the list Resol-fixed leorun4 bugs to be tested. 
> The start is not greyed but it is not enabled. 
> Checked with latest leo build and unagi device v1-train 07/17 build:
> Gecko-2d17cfb
> Gaia-773a197
> 
> Would that be ok as the expected behaviour? Adding ni Ian Barlow

No. We need to either 

a. Use a disabled star graphic (greyed out, like the back button), or 
b. Hide the toolbar altogether, as per patryk's suggestion in comment 11

Comment 22

[Dale Harvey (:daleharvey)]

Just an update, this is resolved fixed, the toolbar is hidden by the patch ^^^