-
Notifications
You must be signed in to change notification settings - Fork 937
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
base: master
Are you sure you want to change the base?
Add social profile links #5439
Conversation
46f7da6
to
55c89e6
Compare
Do we need another set of icons in addition to |
app/models/social_link.rb
Outdated
|
||
def parsed | ||
URL_PATTERNS.each do |platform, pattern| | ||
username = url.match(pattern) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it more intuitive? Now it suggests there are multiple names in this variable.
app/assets/javascripts/user.js
Outdated
`); | ||
|
||
socialLinkForm.find("button").click(function () { | ||
$(this).parent().remove(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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 |
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). |
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 |
a1b03f9
to
bfa37a0
Compare
bfa37a0
to
dc9d0cb
Compare
dc9d0cb
to
8263827
Compare
Merge conflict was fixed. |
<div class="social-link-fields row mb-3"> | ||
<%= social_link_form.text_field :url, :hide_label => true, :wrapper_class => "col-sm-8" %> | ||
<%= social_link_form.check_box :_destroy, :wrapper_class => "d-none social_link_destroy" %> | ||
<%= social_link_form.label :_destroy, t(".social_links.remove"), :class => "btn btn-outline-primary col-sm-1 align-self-start" %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
col-sm-1
is too narrow
class SocialLink < ApplicationRecord | ||
belongs_to :user | ||
|
||
validates :url, :format => { :with => %r{\Ahttps?://.+\z}, :message => I18n.t("profiles.edit.social_links.http_parse_error") } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
model is referring to controller's i18n key here
@@ -40,6 +40,21 @@ | |||
</div> | |||
</fieldset> | |||
|
|||
<fieldset class="mb-3"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the link editor placed between avatar and home location? If you don't change the order of other things on this page, this place makes the least sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do you think is a good place for it? Should it be between the description and avatar, before the description or the last one?
<p class="text-body-secondary"> | ||
<%= image_tag "/assets/social_link_icons/#{social_link.parsed[:platform].nil? ? 'other' : social_link.parsed[:platform]}.svg", | ||
:alt => social_link.parsed[:platform].nil? ? "other" : social_link.parsed[:platform], | ||
:class => "me-2" %> | ||
<%= link_to social_link.parsed[:name], social_link.url, :class => "text-body-secondary", :rel => "nofollow me" %> | ||
</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<div class="my-3"> | ||
<%= link_to t(".edit_profile"), edit_profile_path, :class => "btn btn-outline-primary" %> | ||
</div> | ||
<% end %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The placement of this button suggests that it leads to editing only the social links. If it actually did, that could have been a good idea, given how many editable things there are already on profile/edit
.
:tiktok => %r{tiktok\.com/@([a-zA-Z0-9_]+)}, | ||
:twitch => %r{twitch\.tv/([a-zA-Z0-9_]+)}, | ||
:twitter_x => %r{(?:twitter|x)\.com/([a-zA-Z0-9_]+)}, | ||
:vimeo => %r{vimeo.com/([a-zA-Z0-9_]+)}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unescaped .
|
||
def parsed | ||
URL_PATTERNS.each do |platform, pattern| | ||
names = url.match(pattern) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
matches urls like https://evil.example.com/reddit.com/user/NotRedditor
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:
Currently parser parses these websites and adds their icons which are taken from Bootstrap-Icons:
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:
Logged in:
Edit Profile page: