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 profile links #5439

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nertc
Copy link
Contributor

@nertc nertc commented Dec 23, 2024

This PR addresses "Please make contact information fields, how to contact the user, email address, telegrams, various social networks https://www.openstreetmap.org" issue mentioned in the #2245

Description

This PR adds:

  • DB: Social links table, which contains social links added by the user
  • Model: Parser to differentiate different kind of websites
  • Controller: Logic for CRUD operations for social links
  • View: Functionality to show, edit, add and delete social links
  • JS: Was only used for adding and removing fields on the user side when they interact to the social links fields

Currently parser parses these websites and adds their icons which are taken from Bootstrap-Icons:

  • Discord
  • Facebook
  • Github
  • Gitlab
  • Instagram
  • Linkedin
  • Line
  • Mastodon
  • Medium
  • Quora
  • Reddit
  • Skype
  • Slack
  • Snapchat
  • Stack Overflow
  • Strava
  • Substack
  • Telegram
  • Threads
  • Tik-Tok
  • Twitch
  • X (Twitter)
  • Vimeo
  • WhatsApp
  • YouTube

Discord, Line, Skype and Slack doesn't show username in the URL. To avoid showing some Id numbers on the profile page, for these cases instead of the Id only the name of the application is shown.
Applications other than those mentioned in the list show their URL on the profile page and have general web icon (globe icon).
If there is an idea that some of them should be removed or others should be added, feel free to share recommendations.

Parser was done in the Ruby to avoid adding more JS client-side logic to the website. If it is preferable, it can be moved to JS.

How has this been tested?

There are validation and functional tests written to test the functionality. In addition to this, different kind of manual testing was done to ensure that all the icons, errors, fields and etc. were displayed correctly.

Screenshots

Logged out:
image
image

Logged in:
image

Edit Profile page:
image
image

@nertc nertc mentioned this pull request Dec 23, 2024
5 tasks
@nertc nertc force-pushed the issue_2245_add_social_links branch 2 times, most recently from 46f7da6 to 55c89e6 Compare December 24, 2024 07:08
@nertc nertc marked this pull request as ready for review December 24, 2024 07:19
@AntonKhorev
Copy link
Collaborator

Do we need another set of icons in addition to app/assets/images/social_icons?


def parsed
URL_PATTERNS.each do |platform, pattern|
username = url.match(pattern)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not a username, is it?

Copy link
Contributor Author

@nertc nertc Dec 27, 2024

Choose a reason for hiding this comment

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

It's a user's name shown in the URL of some websites' profile page (for example, in the https://github.com/nertc nertc is a username)

Copy link
Collaborator

Choose a reason for hiding this comment

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

MatchData object is not a username.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To use more intuitive name and not to confuse "user's name" to username, name of the variable username was changed to names

`);

socialLinkForm.find("button").click(function () {
$(this).parent().remove();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I press this button, this removes the link, and I'd think it's saved. But actually not, failing the validation will resurrect the link.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On document load now application checks and hides those fields that have "_destroy" checkbox checked. Therefore, this problem was solved.

class SocialLink < ApplicationRecord
belongs_to :user

validates :url, :presence => true, :format => { :with => URI::DEFAULT_PARSER.make_regexp(%w[http https]), :message => I18n.t("profiles.edit.social_links.http_parse_error") }
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't check if url starts with http://.

And if you also disable CSP:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed :with to regex pattern that will check the string to start with http:// or https://

@AntonKhorev
Copy link
Collaborator

When I'm making a site for myself and there's this kind of UI with Add buttons adding an input and Remove buttons next to added inputs, I don't bother doing it. I just put a <textarea>, it's easier to edit. In this case I'd have a textarea with each nonempty line corresponding to a link. Why don't people do this? Is it too geeky?

@nertc
Copy link
Contributor Author

nertc commented Jan 8, 2025

When I'm making a site for myself and there's this kind of UI with Add buttons adding an input and Remove buttons next to added inputs, I don't bother doing it. I just put a <textarea>, it's easier to edit. In this case I'd have a textarea with each nonempty line corresponding to a link. Why don't people do this? Is it too geeky?

Thanks for the review. I agree with you, if I did a website only for me, it would be a quicker solution to add a textarea and it would do the job. But for most users current solution will be more intuitive and appealing in terms of UI/UX. It's more common and therefore understandable approach (for example GitHub has the same approach on the profile page).

@nertc
Copy link
Contributor Author

nertc commented Jan 8, 2025

Do we need another set of icons in addition to app/assets/images/social_icons?

They have different design. They are more colorful and more like logos which should gain users' attention (for example as share buttons), but icons added by this PR are one color, simple icons, which should not gain much attention as they are there only for the information for the small part of the profile page (not big functional buttons). We can use icons from the app/assets/images/social_icons, but it will make social profile links too colorful and too much attention grabbing. Therefore, I think, it is better to stick to the new icons.

@nertc nertc force-pushed the issue_2245_add_social_links branch from 55c89e6 to a1b03f9 Compare January 8, 2025 10:22
@nertc nertc force-pushed the issue_2245_add_social_links branch from a1b03f9 to bfa37a0 Compare January 8, 2025 10:53
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.

2 participants