Conversation
WalkthroughConverts PreviewPopUp SCSS from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
rjmunro
force-pushed
the
rjmunro/fix-scss-warinings
branch
from
dbc6d84 to
594929c
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/webui/src/client/ui/PreviewPopUp/PreviewPopUp.scss`:
- Around line 47-49: The CSS rule for the modifier selector &--hidden in
PreviewPopUp.scss uses an invalid value `visibility: none`; update that
declaration to a valid value such as `visibility: hidden` (or `display: none` if
you want it removed from layout) so the PreviewPopUp's hidden state works
correctly; change the single `visibility` line inside the `&--hidden` block
accordingly.
The CSS was wrong and it wasn't used. We unmount the component when we don't want to see it, which is probably a much better choice for this component.
rjmunro
force-pushed
the
rjmunro/fix-scss-warinings
branch
from
594929c to
c9e6ccd
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
| scss: { | ||
| // Silence deprecation warnings from Bootstrap and other libraries | ||
| // These are caused by Bootstrap 5.x not yet fully supporting Dart Sass 2.x | ||
| silenceDeprecations: ['import', 'global-builtin', 'color-functions', 'mixed-decls'], |
There was a problem hiding this comment.
should this be quietDeps: true, instead? that seems to hide all the warnings, and might be better as it wont risk us adding any new silenced deprecations to our code?
but I'm not seeing any warnings from the other change you made, so maybe this hides too much too?
nytamin
left a comment
nytamin
left a comment
There was a problem hiding this comment.
I'm not an expert, but LGTM :)
3 participants
About the Contributor
Type of Contribution
This is a Code improvement
Current Behavior
A lot of SCSS warnings are shown, e.g. when running
yarn devon sofie-core, mostly due to deprecation of certain SCSS features.New Behavior
Warnings from external libraries are suppressed; a few other warnings have been eliminated by changing the code where the changes are small. Many warnings still remain, mostly due to use of
@importrather than@usebut these are a bit harder to fix.I have also removed a redundant and broken
hiddenfeature from the PreviewPopup. When we hide the popup, we unmount it in React, we don't use the hidden feature. It wouldn't have worked anyway because use usedvisibility: none, which is not valid css, it's a confusion ofvisibility: hiddenanddisplay: nonewhich are similar but slightly different concepts.Testing
Affected areas
This affects webui.
Time Frame
Other Information
Status
Summary
This PR addresses SCSS deprecation warnings that appear during development, particularly from Bootstrap 5.x and Dart Sass 2.x compatibility issues. The changes include a combination of code modernization and build-level suppression strategies.
Key Changes
vite.config.mts
silenceDeprecations: ['import', 'global-builtin', 'color-functions', 'mixed-decls']PreviewPopUp.scss
@importto@usefor modern SCSS syntax (@use '../../styles/itemTypeColors')@include item-type-colors()to the namespaced form@include itemTypeColors.item-type-colors()&--hiddenmodifier block that used invalid CSSPreviewPopUp.tsx
hidden?: booleanprop from the component's public interfacepreview-popUp--hiddenconditional CSS class from the classNames objectRationale
The PR acknowledges that many SCSS warnings remain unresolved (primarily due to continued use of
@importinstead of@usethroughout the codebase), but tackles the most impactful issues: suppressing third-party library deprecation warnings at the build level and eliminating the invalid CSS usage in the PreviewPopUp component. The removal of the unused hidden prop aligns with React best practices by eliminating unused component state and code paths.