Closed Bug 1078174 Opened 10 years ago Closed 9 years ago

[Contact] Improve UI on contact settings

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(feature-b2g:2.2+)

RESOLVED FIXED
2.2 S1 (5dec)
feature-b2g 2.2+

People

(Reporter: fang, Assigned: pivanov)

References

Details

Attachments

(11 files, 4 obsolete files)

Attached image 2014-10-06-14-41-11.png
The switch button should be right align with the edge of action button. Currently the switch button is way too left. Thanks!
Blocks: 1069288
Attached file patch for Gaia/master
Attachment #8501015 - Flags: ui-review?(fshih)
Comment on attachment 8501015 [details] [review]
patch for Gaia/master

Looks good! Thanks for the help! : )
Attachment #8501015 - Flags: ui-review?(fshih) → ui-review+
Attachment #8501015 - Flags: review?(francisco)
Attached image settings_ui_contact.png
Hi Pavel, 
I also have other adjustments like to change, do you think you can help? 

Things like:

- The switch button is not vertically centered, Sorry didn't find out before. The button currently placed in a higher position of the list. I would like to make it vertically centered, so it can match with our settings app. And also for some reason the little circle of the switch button is not vertically centered either : ( It looks lower and more stick to the edge. Can we make sure this is following the building block components?  

- Currently the sync Facebook friend section is not following setting UI as well. Can we have it align with Setting app? The Facebook icon vertically centered and so as switch button. And for the total line height set to 6rem as settings app.  

- The spacing between "Set ICE contact" & "delete contacts" button need to be reduced to 1.5 rem 

Please let me know if you need any other information, Thanks!!
Comment on attachment 8501015 [details] [review]
patch for Gaia/master

Delegating review to Jose, since he has been working int that settings screen longer than me.
Attachment #8501015 - Flags: review?(francisco) → review?(jmcf)
Comment on attachment 8501015 [details] [review]
patch for Gaia/master

Fang, Pavel

from the attached screen I cannot see any of the requested changes implemented. Please check the patch, the specs or both

thanks
Attachment #8501015 - Flags: review?(jmcf)
Flags: needinfo?(fshih)
(In reply to Jose Manuel Cantera from comment #6)
> Comment on attachment 8501015 [details] [review]
> patch for Gaia/master
> 
> Fang, Pavel
> 
> from the attached screen I cannot see any of the requested changes
> implemented. Please check the patch, the specs or both
> 
> thanks

Hi Jose, From the first spec, I wanted to move the switch button to be right align with the edge of action button. I installed the patch, it's already been fixed.  And for the second spec, I still need Pavel's help to work on that. Thanks!
Flags: needinfo?(fshih)
(In reply to Fang Shih [:grasspizza] from comment #7)
> (In reply to Jose Manuel Cantera from comment #6)
> > Comment on attachment 8501015 [details] [review]
> > patch for Gaia/master
> > 
> > Fang, Pavel
> > 
> > from the attached screen I cannot see any of the requested changes
> > implemented. Please check the patch, the specs or both
> > 
> > thanks
> 
> Hi Jose, From the first spec, I wanted to move the switch button to be right
> align with the edge of action button. I installed the patch, it's already
> been fixed.  And for the second spec, I still need Pavel's help to work on
> that. Thanks!

thanks for the update Fang. then I will wait for a new version of Pavel's patch
This looks bad. Please fix
blocking-b2g: --- → 2.1?
Whiteboard: [Tako_Blocker]
(In reply to Fang Shih [:grasspizza] from comment #3)
> Created attachment 8501562 [details]
> settings_ui_contact.png
> 
> Hi Pavel, 
> I also have other adjustments like to change, do you think you can help? 
> 
> Things like:
> 
> - The switch button is not vertically centered, Sorry didn't find out
> before. The button currently placed in a higher position of the list. I
> would like to make it vertically centered, so it can match with our settings
> app. And also for some reason the little circle of the switch button is not
> vertically centered either : ( It looks lower and more stick to the edge.
> Can we make sure this is following the building block components?  
> 
> - Currently the sync Facebook friend section is not following setting UI as
> well. Can we have it align with Setting app? The Facebook icon vertically
> centered and so as switch button. And for the total line height set to 6rem
> as settings app.  
> 
> - The spacing between "Set ICE contact" & "delete contacts" button need to
> be reduced to 1.5 rem 
> 
> Please let me know if you need any other information, Thanks!!

Hi Pavel, 
Do you think you have time to help on fixing the issues of comment 3 ? : ) 
Thanks!
Flags: needinfo?(pivanov)
this needs more discussions and will need further design changes. it wont be ready for 2.1 and it will first land in master. 

perhaps we can fix this for tako via devices team after it lands in master.
blocking-b2g: 2.1? → -
Attached image Shot (obsolete) —
Hey Fang,

I fixed few of the problems but I need more info about Facebook section. Is there any spec or can you explain how do you want to look like? Thanks :)
Flags: needinfo?(pivanov) → needinfo?(fshih)
Whiteboard: [Tako_Blocker]
blocking-b2g: - → 2.2?
This is UI enhancement so how about nominating to feature-b2g:2.2?
ni Wilfred and Carrie. 
Maybe this will be covered by contact redesign.
blocking-b2g: 2.2? → ---
feature-b2g: --- → 2.2?
Flags: needinfo?(wmathanaraj)
Flags: needinfo?(cawang)
(In reply to Wesley Huang [:wesley_huang] from comment #13)
> This is UI enhancement so how about nominating to feature-b2g:2.2?

this is part of a minor redesign
Hi, 

These are small fixes and Fang and Pavel are currently working on it. Thanks!
Flags: needinfo?(cawang)
Attached image Settings_facebook.png
Hi Pavel, 
Attached the spec of facebook area. Thanks for the help! : )
Flags: needinfo?(fshih)
Attached image Shot (obsolete) —
Hey Fang,

I fixed the FB icon because there was a problem with the current image for @1.5x (https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/style/images/f_logo%401.5x.png)
now the @1.5x icon is 45x45 pixels but I think there is a mistake with the font sizes:

"Syn Friends" text is 1.8rem now and you want to be 2.8rem?
"Not enabled yet" text is 1.5rem now and you want to be 2.1rem?

is that correct?

p.s. all positions are like in the spec except these 2 text elements
Flags: needinfo?(fshih)
(In reply to Pavel Ivanov [:ivanovpavel] from comment #18)
> Created attachment 8511044 [details]
> Shot
> 
> Hey Fang,
> 
> I fixed the FB icon because there was a problem with the current image for
> @1.5x
> (https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/
> contacts/style/images/f_logo%401.5x.png)
> now the @1.5x icon is 45x45 pixels but I think there is a mistake with the
> font sizes:
> 
> "Syn Friends" text is 1.8rem now and you want to be 2.8rem?
> "Not enabled yet" text is 1.5rem now and you want to be 2.1rem?
> 
> is that correct?
> 
> p.s. all positions are like in the spec except these 2 text elements

Hi Pavel, 
I was working on @1.5x design, and gave you the @1.5x font size. Sorry about that!  
The correct size should be 
"Sync Friends" text is 1.9 rem
"Note Enabled yet" is 1.4 rem. 

Thank you so much!
Flags: needinfo?(fshih)
Attached image Shot (obsolete) —
Thanks Fang.
No problem :)

is it OK now?
Attachment #8511886 - Flags: ui-review?(fshih)
Comment on attachment 8511886 [details]
Shot

Thanks Pavel!
Attachment #8511886 - Flags: ui-review?(fshih) → ui-review+
Attachment #8501015 - Flags: review?(jmcf)
Comment on attachment 8501015 [details] [review]
patch for Gaia/master

see the attached screenshot. Please test this with a FB account and with :

a/ 1 friend imported
b/ more than friend imported
Attachment #8501015 - Flags: review?(jmcf) → review-
Attached image Shot (obsolete) —
Hey Fang,
maybe we need to reduce the font-size of "friends imported" text or maybe the left padding of the texts, or maybe we need to reduce the facebook logo? or maybe something else?
Flags: needinfo?(fshih)
Hi Pavel, I'll make a mock up and a spec for this kind of status! Coming soon! Thanks!!
Hi Pavel, 

I reduced the padding of the text and the facebook logo. You can refer to the spec I attached. I also would like to change the icon for the "update imported friends". I've discussed with UX, and this is more like a "sync" than "refresh", so I need your help to replace the current icon for this. Thanks! Please let me know if you have any other questions. : )
Flags: needinfo?(fshih)
Attached the icon assets. Thanks!
Attachment #8510179 - Attachment is obsolete: true
Attachment #8511044 - Attachment is obsolete: true
Attachment #8513438 - Attachment is obsolete: true
Attachment #8511886 - Attachment is obsolete: true
Assignee: nobody → pivanov
Attached image Shot
Attachment #8516009 - Flags: ui-review?(fshih)
Comment on attachment 8516009 [details]
Shot

Looks good! Thanks Pavel!! : )
Attachment #8516009 - Flags: ui-review?(fshih) → ui-review+
Attachment #8501015 - Flags: review- → review?(jmcf)
Comment on attachment 8501015 [details] [review]
patch for Gaia/master

Pavel,

There are still visualization problems. How to repro:

 when you just don't import any friends (the switch is checked) and it says No imported friends (out of xxx). 

Please revise it. 

thanks
Attachment #8501015 - Flags: review?(jmcf) → review-
Attached image Shot
Hey Fang,

can you suggest what we need to do?
Attachment #8520630 - Flags: feedback?(fshih)
Comment on attachment 8520630 [details]
Shot

ni? Carrie for UX suggestion.
Flags: needinfo?(cawang)
Attachment #8520630 - Flags: feedback?(fshih) → feedback-
Hi, 

I wonder if there has no friends imported, why should we display the string "(out of xxx)"? I think "No imported friends" is clear enough. I'd suggest removing the brackets part in this case and that will make the layout looks nice. Thanks!
Flags: needinfo?(cawang)
Hey Carrie,

thanks for the info, as I understand we need to do following things:
 - "No friends imported (out of {{total}})" should be "No friends imported"

but we have one more case here:
 - "One friend imported (out of {{total}})" do I need to remove "(out of {{total}})" here?

Thanks in advance
Flags: needinfo?(cawang)
(In reply to Carrie Wang [:carrie] from comment #34)
> Hi, 
> 
> I wonder if there has no friends imported, why should we display the string
> "(out of xxx)"? I think "No imported friends" is clear enough. I'd suggest
> removing the brackets part in this case and that will make the layout looks
> nice. Thanks!

I disagree. Showing the amount of friends it is valuable information for the user, even if the user has not imported any at that time
(In reply to Pavel Ivanov [:ivanovpavel][:pivanov] from comment #35)
> Hey Carrie,
> 
> thanks for the info, as I understand we need to do following things:
>  - "No friends imported (out of {{total}})" should be "No friends imported"
> 
> but we have one more case here:
>  - "One friend imported (out of {{total}})" do I need to remove "(out of
> {{total}})" here?

I wouldn't do that

> 
> Thanks in advance
Hi Pavel, 

Why do we have the case "One friend imported"? I couldn't try it out on my device. If there has friends imported, it will show the string: 
Sync friends
<num>/<total> friends imported
I think it will cover the case that only one contact is imported out of amount of contacts?

In addition, if there has no contacts imported but the switch is on, we will provide the string "No friends imported" with the "Update imported friends" button underneath it. Personally I don't feel it's that necessary to show the amount number at this point, but I can provide an alternative solution. 
We display the string:

Sync friends
<0>/<total> friends imported

and still have that update button underneath it. So we can keep the string in two rows and keep the layout consistent in every case. 
What do you say?

Thanks!
Flags: needinfo?(pivanov)
Flags: needinfo?(jmcf)
Flags: needinfo?(cawang)
(In reply to Carrie Wang [:carrie] from comment #38)
> Hi Pavel, 
> 
> Why do we have the case "One friend imported"?

That happens because of L10N literals configuration

https://github.com/mozilla-b2g/gaia/blob/master/shared/locales/import_contacts/import_contacts.en-US.properties#L38

 I couldn't try it out on my
> device. If there has friends imported, it will show the string: 
> Sync friends
> <num>/<total> friends imported
> I think it will cover the case that only one contact is imported out of
> amount of contacts?
> 
> In addition, if there has no contacts imported but the switch is on, we will
> provide the string "No friends imported" with the "Update imported friends"
> button underneath it. Personally I don't feel it's that necessary to show
> the amount number at this point, but I can provide an alternative solution. 
> We display the string:
> 
> Sync friends
> <0>/<total> friends imported

that may work yes

> 
> and still have that update button underneath it. So we can keep the string
> in two rows and keep the layout consistent in every case. 
> What do you say?
> 
> Thanks!
Flags: needinfo?(jmcf)
Comment on attachment 8501015 [details] [review]
patch for Gaia/master

PR updated

Now we have:
0/{{total}} friends imported
Flags: needinfo?(pivanov)
Attachment #8501015 - Flags: review- → review?(jmcf)
Comment on attachment 8501015 [details] [review]
patch for Gaia/master

Pavel,

We need another round. Please check the latest comments on GH

thanks for the effort, we are closer!
Attachment #8501015 - Flags: review?(jmcf)
Comment on attachment 8501015 [details] [review]
patch for Gaia/master

ouch ... sorry ... mess the git stuff ... I think now is ready for r? ... sorry one more time
Attachment #8501015 - Flags: review?(jmcf)
Pavel,

You have to modify the one friend message as well ...
Attachment #8501015 - Flags: review?(jmcf)
Attachment #8501015 - Flags: review?(jmcf)
Comment on attachment 8501015 [details] [review]
patch for Gaia/master

as per my last comment on GH
Attachment #8501015 - Flags: review?(jmcf) → review-
PR updated
Flags: needinfo?(jmcf)
Please check the latest comment on GH
Flags: needinfo?(jmcf)
Attachment #8501015 - Flags: review- → review?(jmcf)
Comment on attachment 8501015 [details] [review]
patch for Gaia/master

please add the following commit to your patch, TEST it, and then ask me for another review.

https://github.com/jmcanterafonseca/gaia/commit/bdbd18defd02726c39c50ba64f19f39477b19330

thanks
Attachment #8501015 - Flags: review?(jmcf) → review-
nice :) I made few test and works perfect as I see :)
Flags: needinfo?(jmcf)
unit tests are broken with this change. please add this commit and wait for a positive TBPL

https://github.com/jmcanterafonseca/gaia/commit/7cb4aa0804b4d0ced349d9a0ec78eb70ffdae2ca
Flags: needinfo?(jmcf)
Flags: needinfo?(pivanov)
Hey Jose,

The patch works fine. There was few test errors ... I just update the PR and waiting to see all green.
Flags: needinfo?(pivanov)
Pavel, 

Are we ready for landing?, if so, please ask me for a final review. We would need this to land asap as other bug scheduled for this sprint is waiting :) 

thanks!
Flags: needinfo?(pivanov)
Flags: needinfo?(wmathanaraj)
Comment on attachment 8501015 [details] [review]
patch for Gaia/master

I think so ... there is one red test but I think is not from our patches. You can make final review
Flags: needinfo?(pivanov)
Attachment #8501015 - Flags: review- → review?(jmcf)
Comment on attachment 8501015 [details] [review]
patch for Gaia/master

Pavel,

I don't know why but this patch breaks the contacts list layout. See attached screen.
Attachment #8501015 - Flags: review?(jmcf) → review-
Attachment #8530188 - Attachment description: 2014-11-28-10-11-47.png → the contact list and the alphha scroll are broken
fixed ... waiting for green tests ... can you r? again
Flags: needinfo?(jmcf)
Comment on attachment 8501015 [details] [review]
patch for Gaia/master

thanks Pavel, please land once you get a green tree
Flags: needinfo?(jmcf)
Attachment #8501015 - Flags: review- → review+
Landed:

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


(We got a green in gaia-try and it's blocking other bugs)
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S9 (21Nov)
Target Milestone: 2.1 S9 (21Nov) → 2.2 S1 (5dec)
I'm looking at the bug but I don't really understand why the plural form was dropped. Any explanation?

We definitely had locales who had to change the sentence structure to make it work, and use the plural form.
Example: "{{imported}} friends imported out of {{total}}". This won't work without a proper plural form.
Flags: needinfo?(pivanov)
Maybe we need to ask Carrie
Flags: needinfo?(pivanov) → needinfo?(cawang)
Hi, 

I didn't make any change to drop the plural form. I wonder if there has any misunderstanding? Thanks.
Flags: needinfo?(pivanov)
Flags: needinfo?(francesco.lodolo)
Flags: needinfo?(cawang)
(In reply to Carrie Wang [:carrie] from comment #60)
> I didn't make any change to drop the plural form. I wonder if there has any
> misunderstanding? Thanks.

What's this then?
https://github.com/mozilla-b2g/gaia/commit/d3c3e86db4563cfc5c1e4b0409f52a23471030b9#diff-9

A string with plural forms support (6 forms) is now one single string (1 form).
Flags: needinfo?(francesco.lodolo)
We need Pavel to feedback on this. Thanks.
Hey all,

I made this changes based on Jose's opinion. Maybe he can give us more feedback
Flags: needinfo?(pivanov) → needinfo?(jmcf)
We have a problem with the plural particularly in the case of 0 friends imported and 1 friend imported. In that particular cases the string was so long that broke the UI layout defined by Fang and implemented by Pavel. so the best solution to the best of our knowledge was to drop plurals and use only one form for the importation details.
Flags: needinfo?(jmcf)
Marking feature-b2g to reflect the fact that it was included in the branch.
feature-b2g: 2.2? → 2.2+
You need to log in before you can comment on or make changes to this bug.