Skip to content

docs: add SASS integration #5456

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

Closed
wants to merge 1 commit into from
Closed

docs: add SASS integration #5456

wants to merge 1 commit into from

Conversation

eye0fra
Copy link
Contributor

@eye0fra eye0fra commented Mar 16, 2017

add SASS integration for font-awesome

add SASS integration for font-awesome
Copy link
Member

@clydin clydin left a comment

Choose a reason for hiding this comment

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

The existing instructions work with all of the style options. You can actually mix any of the supported style sheet formats with a single app.

@eye0fra
Copy link
Contributor Author

eye0fra commented Mar 17, 2017

Yes I know, they are instruction to how integrate font-awesome with sass in angular-cli, like they have done with bootstrap's wiki

@filipesilva filipesilva requested a review from Brocco March 29, 2017 09:08
Copy link
Contributor

@Brocco Brocco left a comment

Choose a reason for hiding this comment

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

Looks good, please rebase.

@clydin
Copy link
Member

clydin commented May 1, 2017

Note that this works as well (with no additional changes required):

      "styles": [
        "../node_modules/font-awesome/scss/font-awesome.scss",
        "styles.scss"
      ],

@filipesilva
Copy link
Contributor

@clydin it does, but showing this setup also helps in other cases where sass doesn't resolve paths. I think it's useful for that.

filipesilva pushed a commit that referenced this pull request May 8, 2017
add SASS integration for font-awesome

Close #5456
filipesilva pushed a commit that referenced this pull request May 8, 2017
add SASS integration for font-awesome

Close #5456
filipesilva pushed a commit that referenced this pull request May 8, 2017
add SASS integration for font-awesome

Close #5456
@clydin
Copy link
Member

clydin commented May 8, 2017

Understood and agree. My only concern is that the impression may be given that the described method is the required method.

filipesilva pushed a commit that referenced this pull request May 8, 2017
add SASS integration for font-awesome

Close #5456
@filipesilva
Copy link
Contributor

As far as usage goes though, there's absolutely no difference between using the .css and .scss in the assets array afaik. So a user that wanted to do something specific with FA, or use it as part of a single sass global style, is better off this way, no?

@eye0fra
Copy link
Contributor Author

eye0fra commented May 9, 2017

Agree :)

@clydin
Copy link
Member

clydin commented May 9, 2017

My concern is that someone reading this and wanting to use SCSS in their app may very well think that these steps must be followed to use font-awesome and SCSS in general in their app.

Also with the upcoming improvements to url() handling the need for the path variable should hopefully be eliminated. But I suppose that can be eliminated later.

@hansl hansl closed this in b1d0615 May 9, 2017
dond2clouds pushed a commit to d2clouds/speedray-cli that referenced this pull request Apr 23, 2018
add SASS integration for font-awesome

Close angular#5456
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants