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 optional fa_icon_pack property to social_links to allow more Font Awesome icons packs beyond brands #844

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pfirpfel
Copy link

Summary

Extends functionality introduced in #839. Proposal for my issue raised here: #843

Interface changes

Adds an optional property to the config keys under minima:social_links. Will not break existing configurations.

Motivation

Allow users to use icons from Font Awesome beyond the brands icon pack in their social links.

@ashmaroli
Copy link
Member

Hello again, @pfirpfel
I am not entirely in favor of using Liquid's default filter in a for loop owing to potential performance issues with using the theme in a site with numerous posts and pages because of this conditional flow.
(imagine calling :respond_to? for every social-icon entry with fa_icon_pack property set to a non-falsey value for every post / page in the site.. will it be too much of a concern? Maybe, maybe not.)

The one viable solution I see here is to force everyone to set the entire necessary Font Awesome icon classlist:

minima:
  social_links:
    - title: Jekyll repository at GitHub
      icon: "fab fa-github"
      url: "https://github.com/jekyll/jekyll"
    - title: Email Jekyll
      icon: "far fa-envelope"
      url: "mailto:jekyll@example.com"

Note how one can now opt to use fab or far instead of the expanded forms fa-brands and fa-regular respectively if they are familiar with that syntax.


N.B. You need not rebase your branch if you choose to make changes. We use squash merges here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants