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 Speaker Deck author profile link #299

Closed
wants to merge 1 commit into from

Conversation

glennsarti
Copy link

This commit adds the Speaker Deck link to the author profile. As there is no logo for Speaker Deck in the Font, a simple external link icon is used.

This commit adds the Speaker Deck link to the author profile.  As there is no logo for Speaker Deck in the Font, a simple external link icon is used.
@@ -258,6 +258,10 @@ body:hover .visually-hidden button {
color: $stackoverflow-color;
}

.fa-speaker-deck {
#color: $speakerdeck-color;
Copy link

Choose a reason for hiding this comment

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

Why is there a # here? That can't work, can it?

Copy link
Author

Choose a reason for hiding this comment

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

Typo. I'll fix that

@ghost
Copy link

ghost commented May 31, 2016

Thanks for providing this example. I noticed Speaker Deck hasn't been added to FA but voting is taking place here: FortAwesome/Font-Awesome#1688.

@glennsarti
Copy link
Author

@JHabdas Yeah I did see that voting. I also saw that the issue was raised in 2013. And given there's over 3,500 open issues I didn't see this particular icon getting much traction in the near future. Instead I figured on using the external link icon to provide the same functionality.

@mmistakes
Copy link
Owner

Thanks for the PR. I'll keep this open in the off chance others are looking for a way to add supprort. I tend to shy away merging in these types of PRs as I have no interest in adding ever social network under the sun to the author sidebar.

I'm sticking with the popular requests and the fact that Font Awesome doesn't even have an icon for Speaker Deck doesn't help its cause much 😛

@glennsarti
Copy link
Author

@mmistakes Yeah, I agree that it would be difficult to manage the links for EVERY network. Is there perhaps a better way to display arbitrary external links in the profile;

e.g.
In a _config.yml

...
author:
  externallinks:
    speakerdeck: "https://......."
    someothersite: "http://...."
...

And then just render it as list of links in the profile?

<icon> Github
<icon> Twitter
<icon> speakerdeck           <----- The name of the link comes from the YML
<icon> someothersite

@mmistakes
Copy link
Owner

Well the easiest way is hacking up author-profile.html just as you have. If someone wants to include other links or customize the order or any other particulars that's the best way. The theme was meant to be customized after all...

Another option is I bake in something like _includes/custom-author-links.html that forkers can add whatever they want without much fear of diverging from master if they wish to pull in updates later. I did this with the head and footer includes.

There's also support for custom sidebars that could possibly be augmented to drive some of this functionality. I use them in conjunction with a custom nav lists to build the documentation site's sidebar links.

@vg
Copy link

vg commented Jun 14, 2016

If the intention is to use a generic icon like fa-external-link for Speaker Deck, why not have a generic option itself and not limit it to speaker deck. Such that the generic option can be pointed to any url as required.

@mmistakes
Copy link
Owner

mmistakes commented Jun 15, 2016

@vg My intention is not to merge in these sorts of social network link adds and rather leave it to the user to customize the author-profile.html include.

Editing your copy of the author sidebar affords you way more flexibility to rearrange the order and add/remove links anyway you want, rather than rely on whatever defaults I baked into the theme.

They're there to get you started and cover the most common uses.

Also if I include a generic icon to be set with any url I can almost guarantee someone will ask for 2 or 3 or more to accommodate extra links. I'm trying to avoid that because I really don't want to bloat it with conditionals covering every possible icon in the Font Awesome library. The theme is called Minimal Mistakes for a reason.

@vg
Copy link

vg commented Jun 15, 2016

@mmistakes Understood. Makes sense.

@glennsarti
Copy link
Author

@mmistakes Would / Do you need assistance with #362 ?

I'm happy to close this PR with a reference to that issue.

@vg
Copy link

vg commented Jun 15, 2016

@mmistakes No, I have figured out how to customise the theme as per my needs. But I would like to see a write up on how it can be done nicely in the Quick Start Guide. Every change I make leads to edit conflicts which I have to resolve manually to update. That seems to be the only major problem now. Hope Jekyll 4 will improve that.

@mmistakes
Copy link
Owner

@vg Once gemified Jekyll themes matures I'll be able to rebuild the theme that way. Updating and installing will be a 1,000x times easier. Plus you'll be able to override any of the layouts, includes, styles, etc. and not worry about messing with the core theme.

But that seems a bit off so for now the best you can hope for is dealing with a few merge conflicts here and there if you decide to pull in updates.

@mmistakes
Copy link
Owner

@glennsarti I think I have it covered. Now #324 on the other hand... if you can help there that's one I could use some help sorting out. If it's even possible.

@glennsarti
Copy link
Author

Unfortunately I won't be much use on that one. It's waaaaay beyond what I know.

@mmistakes mmistakes closed this in f9d2770 Jun 17, 2016
@glennsarti
Copy link
Author

sobkowiak pushed a commit to sobkowiak/sobkowiak.github.io-new that referenced this pull request Jul 18, 2016
cjmadsen pushed a commit to cjmadsen/cjmadsen.github.io that referenced this pull request Dec 7, 2016
Manu343726 pushed a commit to Manu343726/Manu343726.github.io that referenced this pull request Sep 17, 2018
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.

3 participants