-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
fix: remove unused --plyr-font-smoothing variable and set default smoothing #2879
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
base: master
Are you sure you want to change the base?
Conversation
|
@sampotts Could you please review this PR? Thanks! |
|
Ideally, we'd keep the CSS custom property so folks can still set it somehow? |
|
@sampotts Thanks for the clarification! Just to confirm — would you prefer keeping the CSS custom property I can update the PR so the custom property remains available for users to override, Let me know and I'll push an update! |
|
@AbdelatefElshafei Just out of curiosity, could you elaborate on your changes? It looks as if Edit: To clarify, I meant that in your changed code, the mixin |
@gardiner Hi Ole, Thanks a lot for your detailed feedback! You're absolutely right. I've updated the PR to address your concerns: The SCSS variable $plyr-font-smoothing is still present as the default, so users can override it via CSS or SCSS. The mixin plyr-font-smoothing($mode) now respects the $mode argument — calling @include plyr-font-smoothing(true) will enable font smoothing regardless of the default variable. The default behavior remains consistent, and all elements now apply font smoothing correctly. This should fix the issue you pointed out about the mixin ignoring the argument. Thanks again for reviewing — happy to make any further adjustments if needed! Best, |
|
@AbdelatefElshafei Awesome, thank you! |
|
|
@sampotts Can You Take another look i made some changes to make it better |
| $plyr-font-weight-regular: var(--plyr-font-weight-regular, 400) !default; | ||
| $plyr-font-weight-bold: var(--plyr-font-weight-bold, 600) !default; | ||
| $plyr-line-height: var(--plyr-line-height, 1.7) !default; | ||
| $plyr-font-smoothing: var(--plyr-font-smoothing, false) !default; |
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.
We should keep the default as false.
Link to related issue (if applicable)
Fixes #2873
Summary of proposed changes