-
Notifications
You must be signed in to change notification settings - Fork 25
refactor: move/remove vendor styles #83
refactor: move/remove vendor styles #83
Conversation
fd96baa
to
2e89851
Compare
Codecov ReportBase: 49.02% // Head: 49.12% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #83 +/- ##
==========================================
+ Coverage 49.02% 49.12% +0.09%
==========================================
Files 76 76
Lines 2005 2003 -2
Branches 372 372
==========================================
+ Hits 983 984 +1
+ Misses 988 985 -3
Partials 34 34
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
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.
Thanks @brian-smith-tcril! What a relief to have vendor/
gone!
I only had one question, and I think we should have a separate commit for the minor change in behavior. Otherwise, I tested it and it looks great!
@@ -1,3 +1,8 @@ | |||
@mixin line-height($fontSize: 16) { | |||
line-height: $fontSize + px; | |||
line-height: ($fontSize/10) + rem; |
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.
Just checking: is this a polyfill?
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'm not sure, it was being used in this modal so i moved it from vendor to here
i'll gladly remove it and replace the usages with a simple line-height: 28px
if you'd prefer
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.
Yeah, this looks pretty archaic. Looked at the source in edx-platform, it's nearly 10 years old. Who knows what that was trying to solve for. Let's replace it!
3d787cc
to
69219bc
Compare
69219bc
to
132c28f
Compare
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.
Works great! Approved.
this should cover most of #72