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

Add Medium and LinkedIn to social partial #31

Merged
merged 2 commits into from Oct 25, 2018

Conversation

@RicardoBelchior
Copy link
Contributor

commented Oct 22, 2018

Hi there,

Simple PR to include LinkedIn and Medium accounts into the "social" partial social.html

Cheers,
Ricardo

@sauerj

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2018

Nice contribution!

I think it would be nice to split the changes into different commits. One for each button.

There is no configuration example for Medium in config.toml.example

@AngeloStavrow likes the buttons be alphabetical ordered, just like in #24

Edit: I wrote the comment a little to early, made some reviews.

@RicardoBelchior

This comment has been minimized.

Copy link
Contributor Author

commented Oct 23, 2018

sure , no problem

@RicardoBelchior RicardoBelchior force-pushed the RicardoBelchior:master branch 2 times, most recently from 833a3bb to 3ae9240 Oct 23, 2018

@RicardoBelchior

This comment has been minimized.

Copy link
Contributor Author

commented Oct 23, 2018

should be done, now =) let me know if you want any other change

@AngeloStavrow AngeloStavrow self-requested a review Oct 23, 2018

@AngeloStavrow

This comment has been minimized.

Copy link
Owner

commented Oct 23, 2018

Thanks for opening this PR! I'll have a look at it today.

@AngeloStavrow AngeloStavrow self-assigned this Oct 23, 2018

@AngeloStavrow
Copy link
Owner

left a comment

I've made some comments regarding changes to social.html; otherwise, looks good to me!

@@ -35,6 +35,16 @@
<a class="glyph" alt="Keybase profile" href="https://keybase.io/{{ .Site.Params.KeybaseUser }}"><img src="{{ .Site.BaseURL }}icons/keybase.svg" height="24px" width="24px"></a>
</div>
{{ end }}
{{ with .Site.Params.LinkedInUser }}
<div class="icon-24x24">
<a class="glyph" alt="LinkedIn profile" href="https://www.linkedin.com/in/{{ . }}"><img src="/icons/linkedin.svg" height="24px" width="24px"></a>

This comment has been minimized.

Copy link
@AngeloStavrow

AngeloStavrow Oct 23, 2018

Owner

This is the way I had the social icons set up in the initial release of the theme, with img src="/icons/icon-name.svg". @sauerj pointed out in #27 that if the Hugo site is set up somewhere other than a site's root directory (e.g., at example.com/blog), the icon assets won't be reachable, so I've changed it. For consistency, you can have a look at the way other icons are added in this file.

@sauerj
Copy link
Contributor

left a comment

I noticed some changes which are not directly linked to the PR. Maybe we should fix all trailing whitespaces and missing newlines in a separate PR?

@@ -29,11 +30,11 @@ theme = "indigo"

# These are parameters used for indieweb identity. You should set these along
# with the above email/social network parameters.
[params.indieWeb]
[params.indieWeb]

This comment has been minimized.

Copy link
@sauerj

sauerj Oct 23, 2018

Contributor

@AngeloStavrow in this commit there are some changes which have nothing to do with the PR. Are you okay by merging them? I noticed some trailing whitspaces in some files, but had not the time to fix this. Maybe a new issue is a good idea for this?

This comment has been minimized.

Copy link
@AngeloStavrow

AngeloStavrow Oct 23, 2018

Owner

Opening a new issue is a good idea. I haven't really done a thorough audit of whitespace issues.

In the meanwhile I think it's fine to include these changes with this PR as it's really just a bit of cleanup, and not a change that affects functionality in some unrelated way.

City = "CityName"

This comment has been minimized.

Copy link
@sauerj

sauerj Oct 23, 2018

Contributor

Same for this change as I mentioned above.

@@ -30,11 +31,11 @@ paginate = 3

# These are parameters used for indieweb identity. You should set these along
# with the above email/social network parameters.
[params.indieWeb]
[params.indieWeb]

This comment has been minimized.

Copy link
@sauerj

sauerj Oct 23, 2018

Contributor

Same as above

City = "CityName"

This comment has been minimized.

Copy link
@sauerj

sauerj Oct 23, 2018

Contributor

Same as above.

@sauerj

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2018

@RicardoBelchior sorry I forgot to submit my review this morning. I had the same suggestions as @AngeloStavrow

@RicardoBelchior RicardoBelchior force-pushed the RicardoBelchior:master branch from 3ae9240 to 3e2f4b9 Oct 23, 2018

@RicardoBelchior

This comment has been minimized.

Copy link
Contributor Author

commented Oct 23, 2018

Sorry for the extra-spaces guys , I did wrote this PR without paying much attention^^
I updated the socials to the new format (my forked branch was not up to date).

And I force-pushed once again :) Let me know if you need any other change.

@sauerj

sauerj approved these changes Oct 23, 2018

Copy link
Contributor

left a comment

Looks good to me.

@AngeloStavrow
Copy link
Owner

left a comment

Looks good to me as well!

@AngeloStavrow AngeloStavrow merged commit 582bc0c into AngeloStavrow:master Oct 25, 2018

@RicardoBelchior

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2018

Thanks guys 👍

@AngeloStavrow

This comment has been minimized.

Copy link
Owner

commented Oct 25, 2018

No, thank you, @RicardoBelchior, for your contributions! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.