-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
adding social buttons #816
Conversation
@karlitschek, thanks for your PR! By analyzing the annotation information on this pull request, we identified @rullzer, @nickvergessen and @MorrisJobke to be potential reviewers |
image_path('core', 'twitter.svg'), | ||
image_path('core', 'rss.svg'), | ||
image_path('core', 'mail.svg'), | ||
'<a target="_blank" href="https://plus.google.com/b/104036748063781940910/104036748063781940910/about">', |
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.
Please add rel="noreferrer noopener"
to all external links, otherwise the opening page can control the window
object. See https://mathiasbynens.github.io/rel-noopener/ for more details.
So basically somebody clicks the link to Twitter and Twitter can make the tab where the link has been clicked change to any other URL without you noticing.
Works 👍 Some small nitpicks in the comments above :-) |
looks nice 👍 |
d23215b
to
0047059
Compare
svg's should be optimized for size (forgot the name of the tool @jancborchardt used) |
], | ||
$l->t(' | ||
<p class="social-button"> | ||
{googleopen}<img width="50" src="{googleimage}" title="Follow us on Google Plus!" alt="Follow us on Google Plus!">{linkclose} |
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.
👎 Any specific reason why HTML was put into the translation string? I guess we should only have text in the translated strings, e.g. Follow us on Google Plus!
and then decorate the translated string with the HTML around, e.g. the image tag. Otherwise translators might be confused about this syntax and even mess it up for some languages.
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.
example:
'<p class="social-button">
{googleopen}<img width="50" src="{googleimage}" title="' . $l->t('Follow us on Google Plus!') . '" alt="' . $l->t('Follow us on Google Plus!') . '">{linkclose}'
svgo |
No, the tool is called scour. (svgo is too lossy). The line from core/img/image-optimization.sh can be used. |
Compressed the icons and fixed some CSS. @karlitschek what about @ChristophWurst’s comments above? |
@jancborchardt @ChristophWurst Hehe. Yes. Totally true. I just had no time to fix this yet. Feel free... |
Fixed the translation of HTML. |
👍 |
1 similar comment
👍 |
...we don't want this in Nextcloud 10, right?! @karlitschek |
Awesome guys. Thanks for finishing this! I think this is not critical but the risk for 10 is also low. So feel free to backport if wanted :-) |
I would drop the backport of this. Nothing crucial in my opinion. |
Agreed |
adding social buttons
Adding social buttons to the personal and admin page as planed here: #745