-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Cleanup: Remove wyrm specific icon sass #1063
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this can be updated and merged after https://github.com/readthedocs/sphinx_rtd_theme/pull/1064/files
This pull requests should actually be merged first, I based #1064 on the changes here. |
I guess the question is if we want to wait for a larger release or not |
Got it, I think we should include this together with the other in the same release |
@agjohnson is this change acceptable outside of the wyrm/bootstrap/sphinx2/html4/fontawesome5 changes? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to be too careful here, but removing classes that downstream users might have been using should at very least be a breaking change probably.
@@ -1,24 +0,0 @@ | |||
.icon | |||
@extend .fa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I maybe have slight concern here about removing the class, in case anyone downstream is using the .icon
classes. This might be another backwards incompatible change
I'll target a 1.1 release for now, however I think this probably makes sense in a 2.0 release with a few other backwards incompatible changes before a 3.0 or greater bootstrap release. |
@agjohnson can this go into 1.1? |
Great update, should be merged quickly once we start working on 2.0. We could write alias css classes for |
This removes wyrm specific icon sass and migrates fully to using font awesome.
There should be no difference to users