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

Sharing buttons update #1121

Merged
merged 2 commits into from
Aug 6, 2019
Merged

Sharing buttons update #1121

merged 2 commits into from
Aug 6, 2019

Conversation

Themroc
Copy link
Contributor

@Themroc Themroc commented Aug 6, 2019

G+ and delicious are no more available.

Copy link
Member

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, this was something we had on our to-do list. Could you please address the style issues listed in the Travis check and update the documentation?

Could you also share the source of the Diaspora icon? We will be switching to vector icons soon and having the source could be useful.

this.register('googleplus', 'g', function(url) {
window.open('https://plus.google.com/share?url=' + encodeURIComponent(url));
this.register('diaspora', 'd', function(url, title) {
window.open('https://share.diasporafoundation.org/?url='+encodeURIComponent(url)+'&title='+encodeURIComponent(title)+'&notes='+'');
Copy link
Member

Choose a reason for hiding this comment

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

+ needs to be spaced from both sides.

Copy link
Member

Choose a reason for hiding this comment

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

Also why is '&notes='+'' needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

" + " Oh, really? Works in chromium... but no prob, will fix. And remove the &notes as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw. very nice piece of software! Never seen one where such a change requires only touching one file.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it is just a style preference – we are trying to have an uniform style everywhere and have a scripts to enforce that. If you have the development environment set up, you can use npm run lint:client -- --fix command to convert the code to the preferred style automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Travis was complaining about the "+" vs. " + ". That's fixed with another commit.
Graphics is from here:
https://www.deviantart.com/creativekaizen/art/Diaspora-Icon-Set-257241499
Vector is in .ai-file (Adobe Illustrator, i guess). Inkscape can open & save as .svg. Should i upload that somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The upload here is crap. Doesn't even like a 10-Byte-.txt-file :(

Copy link
Member

Choose a reason for hiding this comment

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

A link should be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jtojnar
Copy link
Member

jtojnar commented Aug 6, 2019

Sorry, I accidentally closed this when rebasing.

Could you please push to your branch again and reopen the PR.

@Themroc
Copy link
Contributor Author

Themroc commented Aug 6, 2019

hmmm... i don't get this github-thingie... a few minutes ago (after your rebase) it said "last commit in may 2019". In my fork... So, i re-git cloned it, applied my patches, github says you force-pushed my branch, but https://github.com/SSilence/selfoss/blob/master/public/js/selfoss-shares.js is still the old version...

@Themroc
Copy link
Contributor Author

Themroc commented Aug 6, 2019

Think i remember the linux-folks having said they don't accept pull requests on github, because "they don't work" ... Best way forward is probably closing this PR, me deleting my fork and do it all over again tomorrow...

@jtojnar
Copy link
Member

jtojnar commented Aug 6, 2019

The pull request work fine (unless I accidentally force push already merged HEAD like current master instead of my rebased HEAD, which closes the PR and removes maintainer’s access to the branch), I just have not merged it yet. Currently testing.

@jtojnar
Copy link
Member

jtojnar commented Aug 6, 2019

It looks like the icon does not really fit in with the other monochrome icons:

image

What do you think about https://wiki.diasporafoundation.org/File:Logo.png?

@Themroc
Copy link
Contributor Author

Themroc commented Aug 6, 2019

Ok, great!
The icon looks good enaugh for me... but yeah, little bit mushy. I'll try to remove that shadow in inkscape.

@jtojnar
Copy link
Member

jtojnar commented Aug 6, 2019

This is how it looks with the new icon:

image

@Themroc
Copy link
Contributor Author

Themroc commented Aug 6, 2019

How's this
http://www.69b.org/diaspora-simple.svg
one?

@jtojnar
Copy link
Member

jtojnar commented Aug 6, 2019

I think I still prefer the black asterisk, as that is what they have on the branding page. The other sharers (except for Facebook and Wordpress) also use positive, rather than negative images.

@jtojnar
Copy link
Member

jtojnar commented Aug 6, 2019

And also, the positive is what the icon set we will most likely use provides:

https://fontawesome.com/icons/diaspora?style=brands

@Themroc
Copy link
Contributor Author

Themroc commented Aug 6, 2019

This
dia
is what a diaspora-pod looks like on a browser's bookmark bar, so i'd rather use white-on-black... But i have to admit, i like your black-on-white one better now...

@jtojnar jtojnar merged commit 54d555c into fossar:master Aug 6, 2019
@jtojnar jtojnar added this to the 2.19 milestone Aug 6, 2019
@jtojnar
Copy link
Member

jtojnar commented Aug 6, 2019

I merged it now, thanks for all your work.

@fossar fossar deleted a comment from Themroc Aug 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants