Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add social sharing #3679

Merged
merged 5 commits into from
Mar 16, 2017
Merged

Add social sharing #3679

merged 5 commits into from
Mar 16, 2017

Conversation

rullzer
Copy link
Member

@rullzer rullzer commented Mar 2, 2017

  • Add socialshare manager
  • Add social share field under link share

Fixes #3368 based on owncloud/core#25576

Basically it introduces a manager (in javascript) to add the social sharing buttons. This makes sure that people that don't want social sharing or want their own fancy social network can just write their own app.

Edit: this is how it looks now:

bildschirmfoto 2017-03-14 um 22 46 31


To see it in action install https://github.com/nextcloud/socialsharingdefault

socialshare

As usualy some CSS wizzard to make it look better is appreciated else we do it like this until somebodies eyes bleed enough. @nextcloud/designers

showLink: true,

events: {
"click .pop-up": 'onPopUpClick'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mixed quotes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

});

OC.Share.ShareDialogLinkSocialView = ShareDialogLinkSocialView;
})();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would make sense to pass OC and any other globals as parameter here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree in general. But I don't wanna mess with the init stuff there right now.

@@ -421,6 +426,9 @@
this.shareeListView.$el = this.$el.find('.shareeListView');
this.shareeListView.render();

this.socialView.$el = this.$el.find('.socialView');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.$('.socialView') would be simpler

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True

@@ -69,6 +70,9 @@
/** @type {object} **/
shareeListView: undefined,

/** @type {object} **/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OC.Share.ShareDialogLinkSocialView?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

@jancborchardt
Copy link
Member

Design-wise, instead of just dumping it down there as buttons (as another product does …), we should properly integrate it.

For that we simply create a menu where the »Copy link« button was. It serves as the so-to-say »Link sharing« options, of which copying is a part as much as sharing to other services:
capture du 2017-03-02 17-36-54

cc @nextcloud/designers

@jancborchardt jancborchardt added the design Design, UI, UX, etc. label Mar 2, 2017
@rullzer
Copy link
Member Author

rullzer commented Mar 3, 2017

@jancborchardt that looks better indeed. Let me see what I can come up with!

* Add socialshare manager
* Add social share field under link share

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@nickvergessen nickvergessen removed their request for review March 3, 2017 11:10
* If there are social sharing buttons move them and the copy action to a
menu
* If there are no social sharing buttons just leave the copy action
where it is directly

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer
Copy link
Member Author

rullzer commented Mar 3, 2017

@jancborchardt now looks like
socialshare

Somebody that actually knows CSS has to make it look nice. But it is in the menu.
When there are no social sharing plugins it is just the same as before.

@tcitworld
Copy link
Member

tcitworld commented Mar 3, 2017

Is the « Share to » part really necessary ? Isn't a line split into the three icons side by side (fb/g+/twitter) enough ?

@Spartachetto
Copy link

Some proposals/suggestions, up to you to evaluate if interesting

  1. if the mail client is present, it could be useful to have the option Share via mail
  2. It would be cool to extend the supported social networks, especially with the decentralized ones (diaspora*, GNU social, ...)
  3. I am wondering if in some contexts (e.g.: a private firm Nextcloud installation) could be required to block the sharing via social media, or to allow only some of them (I know that @jancborchardt loves options 😄 )

@rullzer
Copy link
Member Author

rullzer commented Mar 6, 2017

@tcitworld I don't care. Somebody with more artistic talent than me has to fix that. Maybe I just make it super ugly so somebody has to fix it before the release ;)

@rullzer
Copy link
Member Author

rullzer commented Mar 6, 2017

@Spartachetto The buttons are provided by an app. So people can write an app for any social media or other thing they want. For example if you only want to allow your users to share to instagram you have to (pay somebody to) write a simple app and only enable that.

@oparoz
Copy link
Member

oparoz commented Mar 6, 2017

I'm a bit worried about the very low discoverability of the feature when it' hidden behind the 3 dots.
@jancborchardt Please conduct a few usability tests to validate that it's obvious enough.

@schiessle
Copy link
Member

schiessle commented Mar 6, 2017

It would be cool to extend the supported social networks, especially with the decentralized ones

I agree, we should have at least one network which fits our philosophy, so I added Diaspora for now: nextcloud/socialsharing#1 (because it was easy, we use it already in the personal settings to share your federated cloud id)

@schiessle
Copy link
Member

I'm a bit worried about the very low discoverability of the feature when it' hidden behind the 3 dots.

I think in general this is fine. But I agree that it is hard to discover it for the first time. Imho that's a nice example why a "what's new" notification makes a lot of sense :hides:

@rullzer
Copy link
Member Author

rullzer commented Mar 6, 2017

@schiessle no I won't add that to the default app. Because people are going to be whining that they want to get rid of it. We can write a different app for diaspora (I just combined g+, fb and twitter because people that want social sharing will for 99% want those).

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@MariusBluem
Copy link
Member

I agree with @rullzer, but I think it is already in now 😕 See nextcloud/socialsharing#1

@rullzer
Copy link
Member Author

rullzer commented Mar 6, 2017

Ah well. What is done is done

@schiessle
Copy link
Member

@rullzer I strongly disagree. 99% also use Dropbox, GDrive or icloud. That's not an argument. I would argue that in our community you have a lot Diaspora users and people who strongly disagree with Facebook, G+, Twitter & Co. By presenting only this options we send the wrong signal.

@MorrisJobke
Copy link
Member

@rullzer I strongly disagree. 99% also use Dropbox, GDrive or icloud. That's not an argument. I would argue that in our community you have a lot Diaspora users and people who strongly disagree with Facebook, G+, Twitter & Co. By presenting only this options we send the wrong signal.

Sadly I need to disagree and we will get a change request to remove diaspora from a customer for sure ...

@schiessle
Copy link
Member

Sadly I need to disagree and we will get a change request to remove diaspora from a customer for sure ...

We will see. I expect most customer will remove it completely and will not be disturbed by just one specific social network. How much customers requested to remove Diaspora from the "share my federated cloud ID"?

@tcitworld
Copy link
Member

tcitworld commented Mar 6, 2017

We can write a different app for diaspora (I just combined g+, fb and twitter because people that want social sharing will for 99% want those).

Couldn't we just add an admin settings section to activate/deactivate some of the social shares possibilities ?

EDIT : My idea is to support more than 3 default social share methods with the socialsharingdefault app, like D*, instagram, linkedin, with only fb/twitter/g+ activated. I feel it stupid to have maybe 5 more apps that do nothing more than adding share buttons. And such sharing provider apps would never get updated anyway.
If a share method is really exotic, then let's provide another app.

@MariusBluem
Copy link
Member

MariusBluem commented Mar 6, 2017

Google+, Facebook and Twitter have some kind of the same target group ... Diaspora is however is outstanding - it should be in a separate provider. This is why the social sharing implementation stuff is done via providers - we can split up this into several provider apps.

Especially enterprise users will usually not use diaspora - and may be irritated by this icon 😅 I have to agree!

@schiessle See the upsides of doing this. With doing this, admins can also go the diaspora-only way, and disable Google+, Twitter and Facebook 😉 Everybody wins.

@rullzer
Copy link
Member Author

rullzer commented Mar 7, 2017

So the app is just an example (because I needed more than 1 there). The approach will be basically 1 app per provider. So everybody can create their own social sharing list. This IMO makes a lot more sense then 1 app that holds everything and then we spend time developing a new admin interface to enable certain parts.

@jancborchardt
Copy link
Member

An admin should have one single setting, and that is »enable sharing to social networks«. Then users should have individual settings to enable/disable the individual networks, whichever they use. This is not an admin decision because different users use different networks. Hence it makes no sense as different apps.

@oparoz
Copy link
Member

oparoz commented Mar 8, 2017

Then users should have individual settings to enable/disable the individual networks, whichever they use. This is not an admin decision because different users use different networks.

Yes and we should cap it at 3 or 5 networks to make it manageable in the sharing interface

Hence it makes no sense as different apps.

Actually, it makes total sense. If one app becomes unmaintained or has security issues, it's easy to disable/remove.
We should provide an app skeleton and a naming convention (easy discovery) and I'm sure the community would be keen to release new SocialSharing apps.

The alternative is to have some form of plugin system. Could be "Dump a class here" system since admins would be responsible for enabling the network, but I foresee some serious upgrade issues.

@jancborchardt
Copy link
Member

That’s all fine tech-wise, as long as in a default install of Nextcloud:

  • You (as admin) don’t need to initially enable a whole bunch of apps for the most basic networks such as Facebook and Twitter (those are even default on iOS for example, we could even make the enabled ones language-dependent like Weibo in China)
  • You (as user) can share to social networks via the dropdown in »share link« (possible to disable for the admin)
  • You (as user) can disable individual social networks in your personal settings

@rullzer is that possible with this approach?

@rullzer
Copy link
Member Author

rullzer commented Mar 8, 2017

@jancborchardt No. and very much out of scope for now

@oparoz
Copy link
Member

oparoz commented Mar 8, 2017

the most basic networks such as Facebook and Twitter (those are even default on iOS for example

Don't forget that these are not the default by accident. The companies behind the products pay millions for the product placement.

@Spartachetto
Copy link

First of all, thank to all of you that "made it happen" with your coding!

I am just citing my experience as one of the possible scenarios.
In my experience there are some work related contexts that see some of the social networks as "professional" and some others as "for fun".
I do not want to say that this is right or wrong, yet it happened to me to work in a place in which there was a strong push to use Twitter (and Linkedin), yet Facebook was seen as "unprofessional".
From my experience that was even true between the peers not working in the firm.

So in that professional context the "usual three" were not usual except one.

@schiessle
Copy link
Member

@rullzer: We should consider adding a "share by mail" option. What do you think? This would also make people here happy: #2357 😉

@rullzer
Copy link
Member Author

rullzer commented Mar 14, 2017

@schiessle uhm sure, not that hard 😄

@MorrisJobke
Copy link
Member

I fixed the layout and it now looks like this:

bildschirmfoto 2017-03-14 um 22 46 31

Can we please get this one here get in? This is only the base and has nothing to do with how the providers are hooked in (apps vs sharing setting) and which ones.

👍 from me

Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@LukasReschke LukasReschke merged commit 39afcbd into master Mar 16, 2017
@LukasReschke LukasReschke deleted the socialsharing branch March 16, 2017 22:08
@ChristophWurst
Copy link
Member

i dunno if i'm in the correct thread to post my problems with socialsharing, especially facebook. so if not please feel free to move this post to the correct thread.

Please use the GitHub search to check whether this has been reported at https://github.com/nextcloud/server/issues and if not, please file an issue at https://github.com/nextcloud/server/issues/new. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews design Design, UI, UX, etc. enhancement feature: sharing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants