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

[zen] again: Font Awesome update to v4.7.0 + theme fork with Fork Awesome v1.0.11 #152

Merged
merged 7 commits into from May 10, 2018
Merged

Conversation

@encarsia
Copy link
Contributor

@encarsia encarsia commented May 2, 2018

Hey all,

as proposed in #151 I updated the zen family to FA4 (de9189c) and created a variant (zen-forkawesome) that uses Fork Awesome icon fonts (2433919).

Before messing things up I decided to accomplish that in a new branch so my last PR was closed when deleting the former working branch.

Copy link
Member

@Kwpolska Kwpolska left a comment

Could we please just link to the CDN instead of shipping all this cruft?

@encarsia
Copy link
Contributor Author

@encarsia encarsia commented May 8, 2018

Comprehension question before I mess things up:
if the theme generally loads FA from CDN the USE_CDN variable in conf.py will not have any effect, will it? (because otherwise the files/cruft had to be provided)

@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented May 8, 2018

Yes, ignore USE_CDN and always use one.

('https://getnikola.com', 'About me', 'icon-user'),
('https://twitter.com/getnikola', 'My Twitter', 'icon-twitter'),
('https://github.com/getnikola', 'My Github', 'icon-github'),
('/index.html', 'Home', 'fa-home'),

This comment has been minimized.

@Kwpolska

Kwpolska May 8, 2018
Member

You need the fa prefix as well (eg. fa fa-home).

This comment has been minimized.

@encarsia

encarsia May 9, 2018
Author Contributor

I'd prefer to add the prefix to the arusahni_helper.tmpl to avoid adding it to every navigation item:

<%def name="html_navigation_links()">
[...]
    <li><a href="${url}" title="${text}"><i class="fa ${icon} fa-3x"></i></a></li>
[...]

Is this a valid option?

This comment has been minimized.

@Kwpolska

Kwpolska May 9, 2018
Member

That can work too, although (a) why fa-3x if we don’t need it now? (b) that wouldn’t cut it for font awesome v5 if someone wanted to use it

This comment has been minimized.

@encarsia

encarsia May 9, 2018
Author Contributor

a) an attempt of making use of an existing class element (fa-Xx sizes) instead of 'font-size' declaration in the css...(you started with getting rid of cruft ;))

b) thanks for clarification, I haven't gone in for FA5.

This comment has been minimized.

@Kwpolska

Kwpolska May 9, 2018
Member

Setting font-size in CSS is more flexible than static .fa-2x/3x sizing.

@encarsia
Copy link
Contributor Author

@encarsia encarsia commented May 10, 2018

Changes:

  1. added "fa" prefix to conf.py.sample files as requested
  2. Fork Awesome also loaded via CDN
  3. the Fork Awesome variant now uses main.css of parent theme (zen)

From my perspective I'm done now...(?)

Signed-off-by: Chris Warrick <kwpolska@gmail.com>
@Kwpolska Kwpolska merged commit a2450f2 into getnikola:master May 10, 2018
@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented May 10, 2018

Merged, thanks!

On a side note, the zen theme needs some modernizations to be usable with v8. I’ll work on it (and other themes) on Saturday.

@encarsia encarsia deleted the encarsia:zen_fa4 branch May 11, 2018
@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented May 13, 2018

I upgraded zen to v8 and Font Awesome v5. zen-forkawesome still works (although needs a few more overrides).

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

Successfully merging this pull request may close these issues.

None yet

2 participants