refactor spinners SCSS#27655
Conversation
MartijnCuppens
left a comment
MartijnCuppens
left a comment
There was a problem hiding this comment.
You can't change the modifier classes like that without changing the
| width: $spinner-width-sm; | ||
| height: $spinner-height-sm; | ||
| border-width: $spinner-border-width-sm; | ||
| &.spinner-sm { |
There was a problem hiding this comment.
I would not use the & here, we don't do that for other modifiers, eg .btn-lg:
Line 110 in d96c147
There was a problem hiding this comment.
Also, the class is not correct
| } | ||
|
|
||
| .spinner-border { | ||
| %spinner { |
There was a problem hiding this comment.
Watch out with extends, you idea was probably to generate less code, but with this technique, you've added 2 extra selectors. I would prefer to see a mixin instead, but don't know if @mdo also agrees?
|
I left a reply to your comment in #22960 about this—we won't be changing to an extend based approach as it adds more selectors. Plus, you've changed the selectors here, which are fine as-is with the base class and modifier classes approach. Thanks though :). |
4 participants
Hi all,
latest SCSS code from #22960 did not work for me on spinner
-smclasses.So I went along and refactored a little bit:
%spinnerwhichspinner-borderandspinner-growboth extendspinner-smmodifier class which is used in combination withspinner-borderandspinner-growto make small spinners, this is the way bootstrap treats other classes as well@keyframesto save more byteAny feedback is heavily welcome.
Have a nice weekend (or start in the week)
midzer