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

use font awesome chevrons instead of inline svg #1041

Merged

Conversation

joachimschmidt557
Copy link
Contributor

Description of proposed changes

Have the narrative navigation use chevron icons from font awesome instead of inline svg. auspice already uses font awesome chevrons for the sidebar, so it would make sense to include them here as well.

Icons used:

Copy link
Member

@jameshadfield jameshadfield 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 this! I have a couple of concrete questions and a bigger question.

Changes Requested:

  • There is no longer any padding below the chevron at the bottom of the page (narrative sidebar). This may have been effected before via the SVG path or the viewbox. Please add this padding back in.
  • 2 linting errors introduced by this PR to be fixed (npm run lint). I'm working on a GitHub action to flag this automatically for PRs.

Philosophical Questions:

  • We currently import 42kb (7kb gz) of fa CSS but use it in very few places. Removing fa is identified as a performance improvement in [meta issue] potential reductions in JS size #702. In other projects i've used import { ICON_NAME } from "react-icons/fa"; which (I believe) will only import the required CSS/SVG. Do you have an opinion on which approach is better?

@joachimschmidt557
Copy link
Contributor Author

Personally, I think it is best to use react-icons. That way we can still reap the performance benefits while also reducing clutter. But that's just my personal opinion.

Copy link
Member

@jameshadfield jameshadfield left a comment

Choose a reason for hiding this comment

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

Thanks!

@jameshadfield jameshadfield merged commit a64f9ba into nextstrain:master Apr 15, 2020
@joachimschmidt557 joachimschmidt557 deleted the font-awesome-chevrons branch April 15, 2020 18:37
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.

3 participants