Add social profile links #5439
Add social profile links #5439
Conversation
nertc
force-pushed
the
issue_2245_add_social_links
branch
2 times, most recently
from
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.
It's not a username, is it?
There was a problem hiding this comment.
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.
MatchData object is not a username.
There was a problem hiding this comment.
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.
Is it more intuitive? Now it suggests there are multiple names in this variable.
There was a problem hiding this comment.
It is named names, because it will either be null or array that will contain one element (user's name). Naming it name would cause to interpret it as a string or other single object variable, while naming it names emphasizes that it is an array that contains a name of the user.
app/assets/javascripts/user.js
Outdated
| `); | ||
|
|
||
| socialLinkForm.find("button").click(function () { | ||
| $(this).parent().remove(); |
There was a problem hiding this comment.
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.
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 |
nertc
force-pushed
the
issue_2245_add_social_links
branch
2 times, most recently
from
a1b03f9 to
bfa37a0
Compare
nertc
force-pushed
the
issue_2245_add_social_links
branch
2 times, most recently
from
dc9d0cb to
8263827
Compare
|
Merge conflict was fixed. |
app/views/profiles/edit.html.erb
Outdated
| <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.
col-sm-1 is too narrow
There was a problem hiding this comment.
It was changed to col-sm-2. Increasing button's width further will make it too wide.
app/models/social_link.rb
Outdated
| 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.
model is referring to controller's i18n key here
There was a problem hiding this comment.
Was fixed with the key that is translated outside of the model.
app/views/profiles/edit.html.erb
Outdated
| @@ -40,6 +40,21 @@ | |||
| </div> | |||
| </fieldset> | |||
|
|
|||
| <fieldset class="mb-3"> | |||
There was a problem hiding this comment.
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.
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?
There was a problem hiding this comment.
Editor of the social links was moved below the description, as, I think, description has higher priority and should be displayed higher in the editor, and also social links are positioned next to the description and, therefore, in the editor too, social links should be next to the description.
| <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.
You can get line wraps like this, where the text goes below the icon:
There was a problem hiding this comment.
From now, URL won't wrap and will show ellipses (...) if it doesn't fit.
| <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.
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.
There was a problem hiding this comment.
I also thought about a separate page or in-place editing for social links, but for consistency, preferred to keep it in the way that everything is done on the profile page. I agree with you about changing general behavior of editing profile page, but, I think, it will be better if we do it as a separate issue and PR and it will change not some part of profile/edit but whole behavior to in-place edit. For now, to avoid any confusion from users about the function of the button, I'll move it to where it was before (under the description block).
|
|
||
| def parsed | ||
| URL_PATTERNS.each do |platform, pattern| | ||
| names = url.match(pattern) |
There was a problem hiding this comment.
matches urls like https://evil.example.com/reddit.com/user/NotRedditor
There was a problem hiding this comment.
From now it will check whether regex of the websites directly follows to http://www., https://www., http:// or https://.
nertc
force-pushed
the
issue_2245_add_social_links
branch
3 times, most recently
from
61ac59c to
922a34e
Compare
|
PR was updated according to the comments. Also, master was rebased as a base for this branch. |
|
Thanks for the review |
nertc
force-pushed
the
issue_2245_add_social_links
branch
2 times, most recently
from
75cdfa7 to
698c296
Compare
app/models/social_link.rb
Outdated
| :instagram => %r{\Ahttps?://(?:www\.)?instagram\.com/([a-zA-Z0-9._]+)}, | ||
| :linkedin => %r{\Ahttps?://(?:www\.)?linkedin\.com/in/([a-zA-Z0-9_-]+)}, | ||
| :line => %r{\Ahttps?://(?:www\.)?line\.me/ti/p/([a-zA-Z0-9_-]+)}, | ||
| :mastodon => %r{\Ahttps?://(?:www\.)?mastodon\.social/@([a-zA-Z0-9_]+)}, |
There was a problem hiding this comment.
Once again mastodon is a problem because mastodon.social is just one option, but I'm not sure what the solution is... At the very least en.osm.town should probably be recognised though.
There was a problem hiding this comment.
en.osm.town was added. It will have the same icon as mastodon.social.
nertc
force-pushed
the
issue_2245_add_social_links
branch
4 times, most recently
from
a2918b5 to
6d3d931
Compare
app/models/social_link.rb
Outdated
| :medium => %r{\Ahttps?://(?:www\.)?medium\.com/@([a-zA-Z0-9_]+)}, | ||
| :quora => %r{\Ahttps?://(?:www\.)?quora\.com/profile/([a-zA-Z0-9_-]+)}, | ||
| :reddit => %r{\Ahttps?://(?:www\.)?reddit\.com/user/([a-zA-Z0-9_-]+)}, | ||
| :skype => %r{\Ahttps?://(?:www\.)?join\.skype\.com/invite/([a-zA-Z0-9_-]+)}, |
There was a problem hiding this comment.
www.join.skype.com doesn't exist. Maybe we don't need www everywhere.
There was a problem hiding this comment.
www. was removed from Skype, Slack and WhatsApp invitation links (all three of them were broken if they were used with www.). As I checked, en.osm.town was also broken with www. and, therefore, Mastodon regex was changed to handle that situation accordingly.
| <div class="text-body-secondary d-flex mb-3"> | ||
| <%= image_tag "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 d-block text-truncate", :rel => "nofollow me" %> | ||
| </div> |
There was a problem hiding this comment.
What do you think about this?
| <div class="text-body-secondary d-flex mb-3"> | |
| <%= image_tag "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 d-block text-truncate", :rel => "nofollow me" %> | |
| </div> | |
| <%= link_to social_link.url, :class => "icon-link mw-100 text-body-secondary mb-3", :rel => "nofollow me" do %> | |
| <%= image_tag "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], | |
| :size => "16" %> | |
| <span class="text-truncate"> | |
| <%= social_link.parsed[:name] %> | |
| </span> | |
| <% end %> |
This also requires putting links in unstyled list (<ul class="list-unstyled">), but don't you want them to be a list anyway?
There was a problem hiding this comment.
They are in a list on github for example
There was a problem hiding this comment.
Thanks for the suggestion. It was applied and <div>s were changed with <ul><li> list structure.
app/models/social_link.rb
Outdated
| } | ||
| end | ||
| end | ||
| { :platform => nil, :name => url } |
There was a problem hiding this comment.
Maybe we should remove http(s):// from "names" of generic urls because the sidebar can get quite narrow and then http(s):// is going to be all you're able to see.
There was a problem hiding this comment.
http(s):// is now removed in the list. I was also thinking about removing www.. What do you think about it?
There was a problem hiding this comment.
I always hate if browsers liying about the real url, but here this is probably a good idea.
nertc
force-pushed
the
issue_2245_add_social_links
branch
from
6d3d931 to
4189291
Compare
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:
