1053758 - Update "add to home screen" dialog to meet new interaction and UX specs

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Ben Francis [:benfrancis]]

Interaction spec: https://mozilla.app.box.com/s/lbw2wzw3p4jvxs24k4sg/1/2099951272/19877711201/1 (page 26)

Visual spec: https://mozilla.app.box.com/s/wj69q6eqgbxb5i8yogqo

Comment 1

[Kevin Grandon :kgrandon]

Peter, Eric - I'm assuming that we use the same dialog for add and edit cases, right? Currently we have the url field there as well and it's editable, are we making a conscious decision to remove it?

Comment 2

[Peter Dolanjski [:pdol]]

(In reply to Kevin Grandon :kgrandon from comment #1)
> Peter, Eric - I'm assuming that we use the same dialog for add and edit
> cases, right? Currently we have the url field there as well and it's
> editable, are we making a conscious decision to remove it?

Good question.  +Francis since he didn't have the url field in the interaction spec either.

Comment 3

[Francis Djabri [:djabber]]

Yes, this was a conscious decision. I don't really feel that the URL field was necessary so the design felt simpler without it.

Comment 4

[Kevin Grandon :kgrandon]

Cristian - would you have some cycles to help us with this dialog UX? Thanks!

Comment 5

[Cristian Rodriguez (:crdlc)]

Attachment: Add link

Comment 6

[Cristian Rodriguez (:crdlc)]

Attachment: Add link with keyboard

Comment 7

[Cristian Rodriguez (:crdlc)]

Attachment: Edit link

Comment 8

[Cristian Rodriguez (:crdlc)]

Attachment: Edit link with keyboard

Comment 9

[Cristian Rodriguez (:crdlc)]

Francis,

   I would like to show you the screenshots (add and edit UI) in order to know the correct titles, to confirm that url field disappears, etc.

Thanks a lot

Comment 10

[Cristian Rodriguez (:crdlc)]

Attachment: Go to PR

Comment 11

[Ben Francis [:benfrancis]]

(In reply to Francis Djabri [:djabber] from comment #3)
> Yes, this was a conscious decision. I don't really feel that the URL field
> was necessary so the design felt simpler without it.

Not being able to edit the URL of a bookmark seems like a bit of a pain and a feature we'd be losing from 2.0 :(

Comment 12

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8482236 [details]
Add link

I know that we need the feedback from Francis for the title but I think that the UI is ready to review

Comment 13

[Kevin Grandon :kgrandon]

Comment on attachment 8482242 [details]
Go to PR

I think it's close, and the code looks good, but it appears there's a perma-failing marionette test. Can you fix browser_chrome_bookmark_web_result_test.js and re-flag me for review? Thanks!

Comment 14

[Francis Djabri [:djabber]]

The visuals seem to follow Eric's spec, but flagging Eric to confirm. 
As for the title, this should be "Add to Homescreen" according to the spec.

Comment 15

[Kevin Grandon :kgrandon]

(In reply to Francis Djabri [:djabber] from comment #14)
> The visuals seem to follow Eric's spec, but flagging Eric to confirm. 
> As for the title, this should be "Add to Homescreen" according to the spec.

Hey Francis - We've actually gotten feedback that we should display "Add to home screen" everywhere from l10n/copy folks. In fact, I suppose "homescreen" isn't a real word and we were requested to use "home screen" in all places instead, specifically lower cased and two words. If you do find yourself in the spec again soon - can you update these references?

Comment 16

[Cristian Rodriguez (:crdlc)]

But what should we show for editing bookmarks? https://bug1053758.bugzilla.mozilla.org/attachment.cgi?id=8482238 "Edit in home screen"?

Comment 17

[Cristian Rodriguez (:crdlc)]

Attachment: Add to home screen

Comment 18

[Cristian Rodriguez (:crdlc)]

Attachment: Edit bookmark

Comment 19

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8482242 [details]
Go to PR

Fixed test Kevin

Comment 20

[Cristian Rodriguez (:crdlc)]

"Rename bookmark", "Rename in home screen",... I need this info before merging, thx

(In reply to Cristian Rodriguez (:crdlc) from comment #16)
> But what should we show for editing bookmarks?
> https://bug1053758.bugzilla.mozilla.org/attachment.cgi?id=8482238 "Edit in
> home screen"?

Comment 21

[Kevin Grandon :kgrandon]

I think keeping either "Edit link" or "Edit bookmark" would make the most sense here.

Comment 22

[Kevin Grandon :kgrandon]

Comment on attachment 8482242 [details]
Go to PR

Let's make sure we hear from UX on the text before landing - thanks!

Comment 23

[Eric Pang [:epang] UX]

Comment on attachment 8483295 [details]
Edit bookmark

Looking at the screen shots this looks good to me!  I think 'Edit bookmark'. Francis do you agree?

Comment 24

[Eric Pang [:epang] UX]

Comment on attachment 8483294 [details]
Add to home screen

Visually looks to spec based on the screen shot.  When flashing the patch I wasn't able to edit the input field and none of the buttons worked, but I'm guessing it's probably my problem.  Thanks!

Comment 25

[Cristian Rodriguez (:crdlc)]

I have changed the string to "Edit bookmark" in the patch

Comment 26

[Cristian Rodriguez (:crdlc)]

Upps I merged without Francis' feedback :( Sorry, I didn't realize. Francis, if you are ok with "Edit bookmark" would be perfect, in another case, I could revert this patch and address your suggest for the string.

Merged in master:

https://github.com/mozilla-b2g/gaia/commit/c26c8a237bc665a9a7e55dd248ab8734fd7d9959

Comment 27

[Francesco Lodolo [:flod]]

Sorry but this needs to be fixed (up to you if you prefer to backout or fix it in a follow-up).
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_best_practices#Changing_existing_strings

Changing case is acceptable, changing strings like this no.

-add-bookmark-header=Add link
-edit-bookmark-header=Edit link
+add-bookmark-header=Add to home screen
+edit-bookmark-header=Edit bookmark

Comment 28

[Kevin Grandon :kgrandon]

Sorry, I left my R+ before this change and didn't catch it. I think Cristian might be gone for the day, so let's backout for now and Cristian can re-land after updating the keys.

Reverted: https://github.com/mozilla-b2g/gaia/commit/6eb0f8fee8d14c0868ad9d4b030dcf8bf90f0141

Comment 29

[Francis Djabri [:djabber]]

(In reply to Cristian Rodriguez (:crdlc) from comment #26)
> Upps I merged without Francis' feedback :( Sorry, I didn't realize. Francis,
> if you are ok with "Edit bookmark" would be perfect, in another case, I
> could revert this patch and address your suggest for the string.
> 
> Merged in master:
> 
> https://github.com/mozilla-b2g/gaia/commit/
> c26c8a237bc665a9a7e55dd248ab8734fd7d9959

Hi Cristian, Edit bookmark is fine with me.

Comment 30

[Kevin Grandon :kgrandon]

Cristian - To clarify, I think you only need to update the l10n keys for the strings and how they're consumed here before landing. They just need to be changed so our l10n teams know they've been updated and can re-translate them. Thanks!

Comment 31

[Cristian Rodriguez (:crdlc)]

working on it

Comment 32

[Cristian Rodriguez (:crdlc)]

Merged in master:

https://github.com/mozilla-b2g/gaia/commit/f818d4f9a6b7f5dd14ea7457220ea5764ff49a5a

Comment 33

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8482242 [details]
Go to PR

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined: users can see how the bookmark will be on the home screen because the icon is displayed while they are bookmarking so it does happier to end users thought
[Testing completed]: Added unit tests
[Risk to taking this patch] (and alternatives if risky): no alternatives
[String changes made]: yes

Comment 34

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/1d5474a788240919b5d15a7f9e0c56d0dd051072