Dropdown options for social media and instant messaging updated, placeholders changed to username #254

xh3n1 · 2017-07-05T18:12:27Z

codecov · 2017-07-05T18:17:12Z

Codecov Report

Merging #254 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #254   +/-   ##
=======================================
  Coverage   15.58%   15.58%           
=======================================
  Files          52       52           
  Lines        1142     1142           
=======================================
  Hits          178      178           
  Misses        964      964

Impacted Files	Coverage Δ
js/services/vCardProperties.js	`18.18% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa28aa5...25437b9. Read the comment docs.

MariusBluem · 2017-07-07T04:44:40Z

js/services/vCardProperties.js

@@ -131,7 +131,10 @@ angular.module('contactsApp')
 			},
 			options: [
 				{id: 'FACEBOOK', name: 'Facebook'},
+				{id: 'INSTAGRAM', name: 'Instagram'},
+				{id: 'LINKEDIN', name: 'Linkedin'},


It’s spelled LinkedIn 😉

MorrisJobke

Tested and works 👍

MorrisJobke · 2017-07-07T10:08:11Z

js/services/vCardProperties.js

-				{id: 'HOME', name: t('contacts', 'Home')},
-				{id: 'WORK', name: t('contacts', 'Work')},
-				{id: 'OTHER', name: t('contacts', 'Other')}
+				{id: 'IRC', name: t('contacts', 'Irc')},


I would say, that those don't need to be translated. Also I would write "IRC", because it is an abbreviation.

MorrisJobke · 2017-07-07T10:08:36Z

js/services/vCardProperties.js

-				{id: 'OTHER', name: t('contacts', 'Other')}
+				{id: 'IRC', name: t('contacts', 'Irc')},
+				{id: 'SKYPE', name: t('contacts', 'Skype')},
+		    {id: 'TELEGRAM', name: t('contacts', 'Telegram')}


Please use the same indentation as for the other entries. Looks like a space vs tabs mixup. ;)

nickvergessen · 2017-07-07T10:35:30Z

package-lock.json

@@ -0,0 +1,4861 @@
+{


I don't think this is intended?

Or why is this file here all of a sudden?

MorrisJobke · 2017-07-07T12:08:48Z

js/services/vCardProperties.js

-				{id: 'HOME', name: t('contacts', 'Home')},
-				{id: 'WORK', name: t('contacts', 'Work')},
-				{id: 'OTHER', name: t('contacts', 'Other')}
+				{id: 'IRC', name: t('contacts', 'IRC')},


Could you change t('contacts', 'IRC') to 'IRC' only - the translation is not really needed for those strings.

MorrisJobke · 2017-07-07T13:08:03Z

js/services/vCardProperties.js

+				{id: 'IRC', name:  'IRC'},
+				{id: 'SKYPE', name:'Skype'},
+				{id: 'TELEGRAM', name:'Telegram'}
+    	]


"103:2 error Mixed spaces and tabs no-mixed-spaces-and-tabs" <- this is the travis error

jancborchardt · 2017-07-07T13:08:08Z

Awesome! Can you review @nextcloud/contacts @nextcloud/javascript? :)

Also cc @jpvdgiessen @qwertygc as it fixes issues you reported. Please review :)

jpvdgiessen · 2017-07-07T14:07:48Z

js/services/vCardProperties.js

 				{id: 'TWITTER', name: 'Twitter'}
+


Maybe also adding Google+ and Pinterest?

Should be the same options of socialsharing-app (https://github.com/nextcloud/socialsharing) ... So Google+ is a good idea 👍

What about Reddit, Snapchat ? @MariusBluem

GnuSocial, Diaspora, XMPP :)

As said in the other comment let’s first start with the basics, not all and every single service we can think of. :) We can always extend later, but let’s not make this pull request too big.

blizzz · 2017-07-07T14:34:35Z

This "username" thing is visible? I hope it is filled with the user's display name?

jancborchardt · 2017-07-08T20:23:43Z

@xh3n1 maybe check what Google Contacts and Apple Contacts offer in their selection - we should have all those in our list for sure. :) Anything else we can still add in the future.

jancborchardt · 2017-07-10T09:44:00Z

@jonatoni @xh3n1 here are the entries iCloud shows.

For »Social network«:

Only one thing: Let’s add Instagram since that’s very popular.
All entries have »Username« as placeholder, except Twitter which prepends an @ as you can see.

This is for »Instant messaging«:

All entries have »Username« as placeholder.

If those are all in the lists, we can merge the pull request. :)

blizzz · 2017-07-10T10:49:07Z

ICQ is still a thing?

qwertygc · 2017-07-10T17:18:12Z

We can add mastodon ?

blizzz · 2017-07-10T21:30:45Z

@qwertygc cf. #254 (comment)

As said in the other comment let’s first start with the basics, not all and every single service we can think of. :) We can always extend later, but let’s not make this pull request too big.

Was fixed

jancborchardt

Looks good! :)

jancborchardt · 2017-07-12T13:18:19Z

So can we go ahead and merge it like this? Further services can be added in a future pull request. :)

@irgendwie @Henni @skjnldsv does one of you mind giving it a quick check?

Henni · 2017-07-12T14:19:15Z

This should still be addressed: #254 (comment)
Otherwise code looks good (currently not able to actually test it).

jancborchardt · 2017-07-13T10:58:45Z

Whitespace comment of @MorrisJobke @Henni was addressed, merging. :)

Congrats on your first pull request @xh3n1 @jonatoni! :)

jancborchardt · 2017-07-13T11:05:22Z

@xh3n1 @jonatoni I added a comment with the remaining services to add at #130 (comment) :)

MariusBluem previously requested changes Jul 7, 2017

View reviewed changes

xh3n1 changed the title ~~Dropdown options for social media and instant messaging updated~~ Dropdown options for social media and instant messaging updated, placeholders changed to username Jul 7, 2017

xh3n1 requested review from MorrisJobke, jancborchardt and nickvergessen July 7, 2017 10:02

MorrisJobke added the 3. to review Waiting for reviews label Jul 7, 2017

MorrisJobke requested review from Henni and irgendwie July 7, 2017 10:03

MorrisJobke approved these changes Jul 7, 2017

View reviewed changes

MorrisJobke reviewed Jul 7, 2017

View reviewed changes

nickvergessen reviewed Jul 7, 2017

View reviewed changes

xh3n1 force-pushed the dropdown_options branch 3 times, most recently from 15bc8dc to 124cfdc Compare July 7, 2017 11:21

MorrisJobke requested changes Jul 7, 2017

View reviewed changes

xh3n1 force-pushed the dropdown_options branch from 0bc0d47 to 47412ff Compare July 7, 2017 12:53

MorrisJobke approved these changes Jul 7, 2017

View reviewed changes

MorrisJobke reviewed Jul 7, 2017

View reviewed changes

jpvdgiessen suggested changes Jul 7, 2017

View reviewed changes

jancborchardt approved these changes Jul 12, 2017

View reviewed changes

xh3n1 force-pushed the dropdown_options branch from f7e54c3 to 8b67845 Compare July 12, 2017 15:16

Adjust dropdown options of instant messengers and social profile

25437b9

xh3n1 force-pushed the dropdown_options branch from 8b67845 to 25437b9 Compare July 12, 2017 15:21

jancborchardt merged commit 376f2f6 into master Jul 13, 2017

jancborchardt deleted the dropdown_options branch July 13, 2017 10:58

This was referenced Jul 13, 2017

when adding a new instant messaging field, the dropdown options are wrong. #252

Closed

How-to use the Social network, Instant messaging Field? #216

Closed

skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Aug 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dropdown options for social media and instant messaging updated, placeholders changed to username #254

Dropdown options for social media and instant messaging updated, placeholders changed to username #254

xh3n1 commented Jul 5, 2017 •

edited

Loading

codecov bot commented Jul 5, 2017 •

edited

Loading

MariusBluem Jul 7, 2017 •

edited

Loading

MorrisJobke left a comment

MorrisJobke Jul 7, 2017

MorrisJobke Jul 7, 2017

nickvergessen Jul 7, 2017

nickvergessen Jul 7, 2017

MorrisJobke Jul 7, 2017

MorrisJobke Jul 7, 2017

jancborchardt commented Jul 7, 2017

jpvdgiessen Jul 7, 2017

MariusBluem Jul 7, 2017

xh3n1 Jul 7, 2017

blizzz Jul 7, 2017 •

edited

Loading

jancborchardt Jul 10, 2017

blizzz commented Jul 7, 2017

jancborchardt commented Jul 8, 2017

jancborchardt commented Jul 10, 2017

blizzz commented Jul 10, 2017

qwertygc commented Jul 10, 2017

blizzz commented Jul 10, 2017

jancborchardt left a comment

jancborchardt commented Jul 12, 2017

Henni commented Jul 12, 2017

jancborchardt commented Jul 13, 2017

jancborchardt commented Jul 13, 2017

Dropdown options for social media and instant messaging updated, placeholders changed to username #254

Dropdown options for social media and instant messaging updated, placeholders changed to username #254

Conversation

xh3n1 commented Jul 5, 2017 • edited Loading

codecov bot commented Jul 5, 2017 • edited Loading

Codecov Report

MariusBluem Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

MorrisJobke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jancborchardt commented Jul 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blizzz Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blizzz commented Jul 7, 2017

jancborchardt commented Jul 8, 2017

jancborchardt commented Jul 10, 2017

blizzz commented Jul 10, 2017

qwertygc commented Jul 10, 2017

blizzz commented Jul 10, 2017

jancborchardt left a comment

Choose a reason for hiding this comment

jancborchardt commented Jul 12, 2017

Henni commented Jul 12, 2017

jancborchardt commented Jul 13, 2017

jancborchardt commented Jul 13, 2017

xh3n1 commented Jul 5, 2017 •

edited

Loading

codecov bot commented Jul 5, 2017 •

edited

Loading

MariusBluem Jul 7, 2017 •

edited

Loading

blizzz Jul 7, 2017 •

edited

Loading