Closed
Bug 1078174
Opened 10 years ago
Closed 9 years ago
[Contact] Improve UI on contact settings
Categories
(Firefox OS Graveyard :: Gaia::Contacts, defect)
Tracking
(feature-b2g:2.2+)
People
(Reporter: fang, Assigned: pivanov)
References
Details
Attachments
(11 files, 4 obsolete files)
48.94 KB,
image/png
|
Details | |
46 bytes,
text/x-github-pull-request
|
jmcf
:
review+
fang
:
ui-review+
|
Details | Review |
170.80 KB,
image/png
|
Details | |
39.36 KB,
image/png
|
Details | |
99.40 KB,
image/png
|
Details | |
61.18 KB,
image/png
|
Details | |
95.79 KB,
image/png
|
Details | |
8.09 KB,
application/zip
|
Details | |
59.01 KB,
image/png
|
fang
:
ui-review+
|
Details |
52.60 KB,
image/png
|
fang
:
feedback-
|
Details |
68.25 KB,
image/png
|
Details |
The switch button should be right align with the edge of action button. Currently the switch button is way too left. Thanks!
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8501015 -
Flags: ui-review?(fshih)
Reporter | ||
Comment 2•10 years ago
|
||
Comment on attachment 8501015 [details] [review] patch for Gaia/master Looks good! Thanks for the help! : )
Attachment #8501015 -
Flags: ui-review?(fshih) → ui-review+
Assignee | ||
Updated•10 years ago
|
Attachment #8501015 -
Flags: review?(francisco)
Reporter | ||
Comment 3•10 years ago
|
||
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 4•10 years ago
|
||
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 5•10 years ago
|
||
Comment 6•10 years ago
|
||
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)
Reporter | ||
Comment 7•10 years ago
|
||
(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)
Comment 8•10 years ago
|
||
(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
Comment 9•10 years ago
|
||
This looks bad. Please fix
blocking-b2g: --- → 2.1?
Whiteboard: [Tako_Blocker]
Reporter | ||
Comment 10•10 years ago
|
||
(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)
Comment 11•10 years ago
|
||
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? → -
Assignee | ||
Comment 12•10 years ago
|
||
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)
Updated•10 years ago
|
Whiteboard: [Tako_Blocker]
Updated•10 years ago
|
blocking-b2g: - → 2.2?
Comment 13•10 years ago
|
||
This is UI enhancement so how about nominating to feature-b2g:2.2?
Comment 14•10 years ago
|
||
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)
Comment 15•10 years ago
|
||
(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
Comment 16•10 years ago
|
||
Hi, These are small fixes and Fang and Pavel are currently working on it. Thanks!
Flags: needinfo?(cawang)
Reporter | ||
Comment 17•10 years ago
|
||
Hi Pavel, Attached the spec of facebook area. Thanks for the help! : )
Flags: needinfo?(fshih)
Assignee | ||
Comment 18•10 years ago
|
||
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)
Reporter | ||
Comment 19•10 years ago
|
||
(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)
Assignee | ||
Comment 20•10 years ago
|
||
Thanks Fang. No problem :) is it OK now?
Attachment #8511886 -
Flags: ui-review?(fshih)
Reporter | ||
Comment 21•10 years ago
|
||
Comment on attachment 8511886 [details]
Shot
Thanks Pavel!
Attachment #8511886 -
Flags: ui-review?(fshih) → ui-review+
Assignee | ||
Updated•10 years ago
|
Attachment #8501015 -
Flags: review?(jmcf)
Comment 22•10 years ago
|
||
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-
Comment 23•10 years ago
|
||
Assignee | ||
Comment 24•10 years ago
|
||
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)
Reporter | ||
Comment 25•10 years ago
|
||
Hi Pavel, I'll make a mock up and a spec for this kind of status! Coming soon! Thanks!!
Reporter | ||
Comment 26•10 years ago
|
||
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)
Reporter | ||
Comment 27•10 years ago
|
||
Attached the icon assets. Thanks!
Assignee | ||
Updated•10 years ago
|
Attachment #8510179 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8511044 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8513438 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8511886 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → pivanov
Assignee | ||
Comment 29•10 years ago
|
||
Attachment #8516009 -
Flags: ui-review?(fshih)
Reporter | ||
Comment 30•10 years ago
|
||
Comment on attachment 8516009 [details]
Shot
Looks good! Thanks Pavel!! : )
Attachment #8516009 -
Flags: ui-review?(fshih) → ui-review+
Assignee | ||
Updated•10 years ago
|
Attachment #8501015 -
Flags: review- → review?(jmcf)
Comment 31•10 years ago
|
||
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-
Assignee | ||
Comment 32•10 years ago
|
||
Hey Fang, can you suggest what we need to do?
Attachment #8520630 -
Flags: feedback?(fshih)
Reporter | ||
Comment 33•10 years ago
|
||
Comment on attachment 8520630 [details]
Shot
ni? Carrie for UX suggestion.
Flags: needinfo?(cawang)
Attachment #8520630 -
Flags: feedback?(fshih) → feedback-
Comment 34•10 years ago
|
||
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)
Assignee | ||
Comment 35•10 years ago
|
||
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)
Comment 36•10 years ago
|
||
(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
Comment 37•10 years ago
|
||
(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
Comment 38•10 years ago
|
||
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)
Comment 39•10 years ago
|
||
(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)
Assignee | ||
Comment 40•10 years ago
|
||
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 41•10 years ago
|
||
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)
Assignee | ||
Comment 42•10 years ago
|
||
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)
Comment 43•10 years ago
|
||
Pavel, You have to modify the one friend message as well ...
Updated•10 years ago
|
Attachment #8501015 -
Flags: review?(jmcf)
Assignee | ||
Updated•10 years ago
|
Attachment #8501015 -
Flags: review?(jmcf)
Comment 44•10 years ago
|
||
Comment on attachment 8501015 [details] [review] patch for Gaia/master as per my last comment on GH
Attachment #8501015 -
Flags: review?(jmcf) → review-
Assignee | ||
Updated•9 years ago
|
Attachment #8501015 -
Flags: review- → review?(jmcf)
Comment 47•9 years ago
|
||
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-
Assignee | ||
Comment 48•9 years ago
|
||
nice :) I made few test and works perfect as I see :)
Flags: needinfo?(jmcf)
Comment 49•9 years ago
|
||
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)
Updated•9 years ago
|
Flags: needinfo?(pivanov)
Assignee | ||
Comment 50•9 years ago
|
||
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)
Comment 51•9 years ago
|
||
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!
Updated•9 years ago
|
Flags: needinfo?(pivanov)
Updated•9 years ago
|
Flags: needinfo?(wmathanaraj)
Assignee | ||
Comment 52•9 years ago
|
||
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 53•9 years ago
|
||
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-
Comment 54•9 years ago
|
||
Updated•9 years ago
|
Attachment #8530188 -
Attachment description: 2014-11-28-10-11-47.png → the contact list and the alphha scroll are broken
Assignee | ||
Comment 55•9 years ago
|
||
fixed ... waiting for green tests ... can you r? again
Flags: needinfo?(jmcf)
Comment 56•9 years ago
|
||
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+
Comment 57•9 years ago
|
||
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)
Updated•9 years ago
|
Target Milestone: 2.1 S9 (21Nov) → 2.2 S1 (5dec)
Comment 58•9 years ago
|
||
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)
Assignee | ||
Comment 59•9 years ago
|
||
Maybe we need to ask Carrie
Flags: needinfo?(pivanov) → needinfo?(cawang)
Comment 60•9 years ago
|
||
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)
Comment 61•9 years ago
|
||
(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)
Comment 62•9 years ago
|
||
We need Pavel to feedback on this. Thanks.
Assignee | ||
Comment 63•9 years ago
|
||
Hey all, I made this changes based on Jose's opinion. Maybe he can give us more feedback
Flags: needinfo?(pivanov) → needinfo?(jmcf)
Comment 64•9 years ago
|
||
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)
Comment 65•9 years ago
|
||
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.
Description
•