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

chore(ionicons): remove the import of ionicons css #10115

Closed
wants to merge 1 commit into from

Conversation

brandyscarney
Copy link
Member

Short description of what this resolves:

Importing the ionicons.scss file from ionicons was adding css that is not desired. Specifically, the .ion class here: https://github.com/driftyco/ionicons/blob/3.0/dist/scss/ionicons.scss#L29-L41

The .ion class itself isn't added to our icons, but it is extended here: https://github.com/driftyco/ionicons/blob/3.0/dist/scss/ionicons-common.scss#L941

So because of this extend, each icon class was getting this css which caused issues all over the e2e tests and in Ionic apps as well. The rule that is causing the issues is line-height: 1.

For example:

screen shot 2017-01-19 at 3 49 01 pm

Changes proposed in this pull request:

  • Remove node_modules/ionicons/dist/scss from the sass config so that it won't import ionicons.scss from there and instead will import it from src/fonts/
  • Change the import for ionicons-icons and ionicons-variables to include the full path with node_modules, because without ionicons in the includePaths it will throw errors that it can't find it

Ionic Version: 2.x

@brandyscarney
Copy link
Member Author

Closing this, we went a different route instead where we just added a new file with a different name to import from: https://github.com/driftyco/ionic/blob/master/src/themes/ionic.ionicons.scss

Starters, preview app and conference app will need to be updated to import from ionic.ionicons here: https://github.com/driftyco/ionic-conference-app/blob/master/src/theme/variables.scss#L81

@jgw96 jgw96 deleted the chore-ionicons-css branch January 25, 2017 20:55
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.

1 participant