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 functionality #4985

Merged

Conversation

kcne
Copy link
Contributor

@kcne kcne commented Jul 13, 2024

Add social network sharing to diary entry pages

Description:
This PR addresses the issue #1391 by introducing social media sharing functionality for diary entries, allowing users to share posts on platforms like Twitter, Facebook, LinkedIn, Mastodon, Telegram, and Email. It includes required JavaScript to handle opening share URLs in pop-up windows and Ruby helper methods to filter and validate allowed social media sites. The changes also incorporate error logging for invalid sites and missing icons, ensuring robustness.

Implementation details:

  • lib/social_share_button_helper.rb - config, validation & helper functions
  • render_social_share_buttons function inside application_helper.rb responsible for rendering the buttons with helper utilization
  • social_share_button.js - javascript utility functions for opening the popups, generating urls for sharing & handling onclick events on buttons

Screenshots:
Screenshot 2024-07-17 at 16 05 18
Screenshot 2024-07-17 at 16 07 12

Reviews welcome,
Thank you.

@kcne kcne force-pushed the 1391-diary-share-with-social-buttons branch 3 times, most recently from f86274e to dc594d0 Compare July 14, 2024 01:18
@kcne kcne closed this Jul 14, 2024
@kcne kcne reopened this Jul 14, 2024
@kcne kcne force-pushed the 1391-diary-share-with-social-buttons branch 3 times, most recently from 58ac1a5 to 02b6a81 Compare July 15, 2024 11:35
@kcne kcne marked this pull request as ready for review July 15, 2024 12:49
@mmd-osm
Copy link
Contributor

mmd-osm commented Jul 30, 2024

Thank you. Some of the links seem to be a bit dated, e.g. Twitter is now known as X, and links/logos would have to be updated, I suppose.

@kcne kcne force-pushed the 1391-diary-share-with-social-buttons branch 2 times, most recently from bd75443 to 66c48af Compare August 13, 2024 00:53
@kcne
Copy link
Contributor Author

kcne commented Aug 13, 2024

Updated the x icon and link as suggested by @mmd-osm. Also if you think we should include some other socials too, suggestions are welcome. Any chance @tomhughes @gravitystorm @AntonKhorev some of you guys can review this soon?
Thank you.

@talllguy
Copy link

Thanks for the hard work on this! Since we're on the topic of social sharing I thought I'd resurface an idea from 10 years ago that I still think would add value to users of the site. Perhaps the technology hurdles back then are less likely to be hurdles now. #739

@kcne kcne force-pushed the 1391-diary-share-with-social-buttons branch 3 times, most recently from 6669767 to 45949e4 Compare August 15, 2024 11:50
@kcne
Copy link
Contributor Author

kcne commented Aug 15, 2024

Refactored this according to @AntonKhorev review. Please let me know If you think I can further improve this. Thank you!

Copy link
Member

@tomhughes tomhughes left a comment

Choose a reason for hiding this comment

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

Added a few comments inline... Other things to consider are whether to offer some other sites - bluesky and threads being the obvious candidates.

app/assets/javascripts/social_share_button.js Outdated Show resolved Hide resolved
app/views/layouts/_head.html.erb Outdated Show resolved Hide resolved
lib/social_share_button_helper.rb Outdated Show resolved Hide resolved
lib/social_share_button_helper.rb Outdated Show resolved Hide resolved
@kcne kcne force-pushed the 1391-diary-share-with-social-buttons branch 3 times, most recently from d73482b to c55095d Compare August 22, 2024 07:32
@kcne
Copy link
Contributor Author

kcne commented Aug 22, 2024

Thank you for the review!
I've resolved the requested changes, added the translations as requested, and the mailto links should now be opening correctly. Once we round this up and resolve the mastodon thing with @tomhughes I can add bluesky and threads support as a separate commit if you agree.

@kcne kcne force-pushed the 1391-diary-share-with-social-buttons branch 2 times, most recently from 9334366 to ff4b223 Compare August 27, 2024 19:40
@kcne
Copy link
Contributor Author

kcne commented Aug 29, 2024

Found out there is mastodonshare.com that takes text and url query parameters and let's you choose the mastodon instance you want to share on. Updated the code accordingly. Can you please test this and let me know if this solution is okay. @mmd-osm @tomhughes

Updated mastodon link, everything should be working fine now. Can you please check and let me know if anything else can be improved. Thank you.

@kcne kcne requested a review from tomhughes August 29, 2024 16:50
@kcne kcne force-pushed the 1391-diary-share-with-social-buttons branch from ff4b223 to 4efc63d Compare September 17, 2024 06:21
@kcne
Copy link
Contributor Author

kcne commented Sep 17, 2024

As @AntonKhorev suggested, we're now displaying these icons only when users open the full diary entry, as outlined in #5121. Since this is currently used in one place, turbo pagination issues can be handled in a future PR if a use case arises for combining it with turbo pagination. Thank you.

@kcne kcne force-pushed the 1391-diary-share-with-social-buttons branch from 4efc63d to 8ba2316 Compare October 29, 2024 06:39
Copy link
Collaborator

@AntonKhorev AntonKhorev left a comment

Choose a reason for hiding this comment

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

It's fine if we're ok with mastodonshare.com for now, but we can replace it later.

@kcne kcne force-pushed the 1391-diary-share-with-social-buttons branch from 8ba2316 to b02d625 Compare December 17, 2024 14:17
@kcne kcne force-pushed the 1391-diary-share-with-social-buttons branch from b02d625 to 95e1aff Compare December 17, 2024 22:42
@AntonKhorev AntonKhorev merged commit 7e7ede8 into openstreetmap:master Dec 18, 2024
22 checks passed

# Generates a set of social share buttons based on the specified options.
def render_social_share_buttons(opts = {})
sites = opts.fetch(:allow_sites, [])
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of the allow_sites option and the supporting logic that allows filtering? As far as I can tell it's tested but never actually used by any other code?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can probably use it if sharing on some site requires some setting, to filter out that site if it's not set, like if we add Mastodon host preference.

Copy link
Member

Choose a reason for hiding this comment

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

Well obviously we can come up with ways to use it, like adding a configuration option to control which ones was enabled. But given that no such thing currently exists I was wondering we it had been implemented - is there some plan to make use of it in the future?

@AntonKhorev
Copy link
Collaborator

Merged, thanks.

Although we're effectively outsourcing Mastodon host option to mastodonshare.com, it works for anonymous users too. Currently our preferences page is not available for anonymous users but we probably should make it available and save preferences to cookies. We have other preferences like #5201 and color mode settings and we shouldn't add a button for each one.

@gravitystorm
Copy link
Collaborator

anonymous users

I think you mean users who are not logged in - "anonymous users" is the term we use for accounts with user.data_public == false. They are quite rare now since they haven't been permitted for a long time.

As for preferences, I'm happy keeping most preferences available for only logged in users, since account creation is free and we aim to serve our mappers (who by definition have accounts) more than just casual visitors. Storing preferences in cookies does have drawbacks (like cross-device consistency, automatic cookie expiry etc) so it's not my first choice in general.

@AntonKhorev
Copy link
Collaborator

I think you mean users who are not logged in

yes

As for preferences, I'm happy keeping most preferences available for only logged in users, since account creation is free and we aim to serve our mappers (who by definition have accounts) more than just casual visitors. Storing preferences in cookies does have drawbacks (like cross-device consistency, automatic cookie expiry etc) so it's not my first choice in general.

Are you against the entire idea of #1115 or just against the cookies and want preferences stored somewhere else like session? I don't think anyone who is not logged in expects cross-device consistency etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants