Skip to content
This repository was archived by the owner on Jun 18, 2024. It is now read-only.

Conversation

@victoriachan
Copy link
Contributor

Code review only. Please do not merge yet!

Add LinkedIn share link to share button. Ticket: https://projects.torchbox.com/projects/rca-django-cms-project/tickets/942

<li><a href="https://www.facebook.com/sharer/sharer.php?u={{ self.url }}">On Facebook</a></li>
<li><a href="http://twitter.com/share?text={{ self.title }}&url={{ self.url }}&via={{ global_default_twitter_handle }}">On Twitter</a></li>
<li><a href="https://twitter.com/share?text={{ self.title }}&url={{ self.url }}&via={{ global_default_twitter_handle }}">On Twitter</a></li>
<li><a href="https://www.linkedin.com/shareArticle?mini=true&url={{ self.url }}">On LinkedIn</a></li>
Copy link
Member

Choose a reason for hiding this comment

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

I am actually wondering if we should not use |urlencode on URLs. Even if the other ones don't have it. Can you please check this? Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added the urlencode and tested the links now.

<li class="last"><a href="mailto:?subject=Royal College of Art: {{ self.title }}&amp;body=I thought you might be interested in this page on the Royal College of Art site: {{ self.url }}.">By Email</a></li>
<li><a href="https://www.facebook.com/sharer/sharer.php?u={{ self.url|urlencode }}">On Facebook</a></li>
<li><a href="https://twitter.com/share?text={{ self.title|urlencode }}&url={{ self.url|urlencode }}&via={{ global_default_twitter_handle|urlencode }}">On Twitter</a></li>
<li><a href="https://www.linkedin.com/shareArticle?mini=true&url={{ self.url|urlencode }}">On LinkedIn</a></li>
Copy link
Member

Choose a reason for hiding this comment

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

&amp;? Except for that it looks good to me. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I was confused as the original link on that page had &amp;body= instead of just &body=. I have updated it to just & now. Hopefully this is the last change!

Copy link
Member

@tm-kn tm-kn left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @victoriachan!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants