Changing stylesheets to SCSS

h1nk commented 4 years ago

Seeing as this project is already using Webpack it'd be nice to make use of SCSS to allow for more DRY and organized stylesheets. I believe @MrTimscampi has already brought this idea up before. An initial transition to SCSS could be made without any stylesheet refactoring or changes since CSS3 is automatically valid SCSS.

(Edit by MrTimscampi)

Here's a list of the files to convert after merging #862, in order to keep track of progress.

When converting a file, don't hesitate to reorganize everything, try to find dead rules and, if necessary, create more files (variables.scss is one that comes to mind, for example)

[ ] ./src/components/htmlvideoplayer/style.css
[ ] ./src/components/filterdialog/style.css
[ ] ./src/components/actionsheet/actionsheet.css
[ ] ./src/components/youtubeplayer/style.css
[ ] ./src/components/images/style.css
[ ] ./src/components/dialogHelper/dialoghelper.css
[ ] ./src/components/slideshow/style.css
[ ] ./src/components/emby-scrollbuttons/emby-scrollbuttons.css
[ ] ./src/components/alphapicker/style.css
[ ] ./src/components/backdrop/backdrop.css
[ ] ./src/components/headroom/headroom.css
[ ] ./src/components/playback/iconosd.css
[ ] ./src/components/subtitleeditor/subtitleeditor.css
[ ] ./src/components/toast/toast.css
[ ] ./src/components/viewManager/viewContainer.css
[ ] ./src/components/logoscreensaver/style.css
[ ] ./src/components/mediainfo/mediainfo.css
[ ] ./src/components/imageuploader/style.css
[ ] ./src/components/navdrawer/navdrawer.css
[ ] ./src/components/lazyloader/lazyedgehack.css
[ ] ./src/components/userdatabuttons/userdatabuttons.css
[ ] ./src/components/cardbuilder/card.css
[ ] ./src/components/appfooter/appfooter.css
[ ] ./src/components/guide/guide.css
[ ] ./src/components/guide/programs.css
[ ] ./src/components/listview/listview.css
[ ] ./src/components/homesections/homesections.css
[ ] ./src/components/imageeditor/imageeditor.css
[ ] ./src/components/upnextdialog/upnextdialog.css
[ ] ./src/components/multiselect/multiselect.css
[ ] ./src/components/search/searchfields.css
[ ] ./src/components/emby-tabs/emby-tabs.css
[ ] ./src/components/recordingcreator/recordingfields.css
[ ] ./src/components/recordingcreator/recordingcreator.css
[ ] ./src/components/subtitlesync/subtitlesync.css
[ ] ./src/components/loading/loadingLegacy.css
[ ] ./src/components/loading/loading.css
[ ] ./src/components/directorybrowser/directorybrowser.css
[ ] ./src/components/indicators/indicators.css
[ ] ./src/components/playerstats/playerstats.css
[ ] ./src/components/emby-scroller/emby-scroller.css
[ ] ./src/components/formdialog.css
[ ] ./src/components/nowplayingbar/nowplayingbar.css
[ ] ./src/components/remotecontrol/remotecontrol.css
[ ] ./src/themes/light/theme.css
[ ] ./src/themes/dark/theme.css
[ ] ./src/themes/purplehaze/theme.css
[ ] ./src/themes/appletv/theme.css
[ ] ./src/themes/blueradiance/theme.css
[ ] ./src/themes/wmc/theme.css
[x] ./src/assets/css/fonts.css
[ ] ./src/assets/css/metadataeditor.css
[x] ./src/assets/css/clearbutton.css
[x] ./src/assets/css/scrollstyles.css
[x] ./src/assets/css/ios.css
[ ] ./src/assets/css/dashboard.css
[x] ./src/assets/css/fonts.sized.css
[ ] ./src/assets/css/librarybrowser.css
[ ] ./src/assets/css/videoosd.css
[x] ./src/assets/css/site.css
[x] ./src/assets/css/livetv.css
[x] ./src/assets/css/detailtable.css
[x] ./src/assets/css/flexstyles.css
[ ] ./src/elements/emby-progressring/emby-progressring.css
[ ] ./src/elements/emby-select/emby-select.css
[ ] ./src/elements/emby-toggle/emby-toggle.css
[ ] ./src/elements/emby-textarea/emby-textarea.css
[ ] ./src/elements/emby-button/emby-button.css
[ ] ./src/elements/emby-checkbox/emby-checkbox.css
[ ] ./src/elements/emby-input/emby-input.css
[ ] ./src/elements/emby-collapse/emby-collapse.css
[ ] ./src/elements/emby-radio/emby-radio.css
[ ] ./src/elements/emby-slider/emby-slider.css

dkanada commented 4 years ago

I believe we will need to finish the transition to Webpack before we can use SCSS in the source, but that's already getting done so it might not be too long.

heyhippari commented 4 years ago

Dependent on #681

JustAMan commented 4 years ago

Should be dependent on https://github.com/jellyfin/jellyfin-web/pull/862 now, right @MrTimscampi?

heyhippari commented 4 years ago

Yep, I need to go through the issues to see what I can mark as fixed in #862

h1nk commented 4 years ago

@MrTimscampi now that #862 is merged into master is it possible for the transition to be made now? I'd like to tackle the initial refactoring to SCSS but I'm not familiar with how to the development pipeline works (Webpack dev server, yarn, Gulp etc.). Could you describe how to start working with SCSS under the new build/dev pipeline?

heyhippari commented 4 years ago

@h1nk It is ready to be started, indeed!

The relevant lies for Gulp are here: https://github.com/jellyfin/jellyfin-web/blob/master/gulpfile.js#L82-L90

Basically, use git mv to move files around if needed (to be in .scss for example), then you do your changes in the files.

You can use yarn serve to open a dev server and look at your changes, it will reload things as you make changes.

h1nk commented 4 years ago

@MrTimscampi, hey thanks for the reply.

I did try this initially as you described but SCSS wasn't being picked up and compiled, here for example a SASS comment is being left in the stylesheet:

I basically ran:

> yarn serve
> git mv .\src\assets\css\site.css .\src\assets\css\site.scss
> # Edit the files accordingly...

heyhippari commented 4 years ago

@h1nk This works for me: https://github.com/jellyfin/jellyfin-web/commit/ae82957d798dfe53e717d5253424591104ad7fb5

You might need to move the files first and we might need to adjust how Browsersync watches files, but it does compile and the dark theme still looks normal.

GuilhermeHideki commented 4 years ago

Hi! I was looking into the files and would like to ask somethings

Can we use CSS variables?

:root {
--subtitle-color: white;
}
.videoSubtitles {
color: var(--subtitle-color);
}

What about creating mixins, like for position?

if we create these mixins, where it should live? /src/stylesheets/mixins.scss?

@mixin position($position, $top: null, $right: null, $bottom: null, $left: null) {
position: $position;
top: $top;
right: $right;
bottom: $bottom;
left: $left;
}

What do you think about using the ampersand like this?
```
.videoSubtitles {
&Inner {
}
}
```

heyhippari commented 4 years ago

Hi @GuilhermeHideki.

We do want to make use of CSS variables, as it makes a lot of things nicer and easier (themes for example).

However, due to targeting some really old browsers (Chrome 27 is our oldest, afaik) for Smart TV compatibility, we'll need to use this ponyfill to enable compatibility.
As such, when using them, we need to make sure to keep the limitations of that ponyfill in mind:

Custom property declaration support is limited to :root and :host rulesets
The use of var() is limited to property values

So feel free to use them :smiley:

For mixins, absolutely. If you notice something that is done often, absolutely make use of mixins to reduce the amount of CSS we have.

And for ampersands, I'm all for using them. So once again, feel free to do so.

As for the folder, we have three types of style folders at the moment:

src/assets/css contains "general" styles that probably require the most cleanup. Much of the global UI styles are defined there, instead of in per-view or per-component files.
In component-specific files
src/themes which contains the various themes (A lot of duplicate lines in there that can probably be cleanup up).

In addition to that, we still have a bunch of inline styles that we'd like to clean up (but haven't taken the time to do so yet).

We're open to changing that structure, but themes should likely stay separate.

GuilhermeHideki commented 4 years ago

Is configuring stylelint for scss code a blocker for this issue?

The .stylelintrc warns about the '@include' (and other sass constructs)
- I guess something like https://github.com/kristerkari/stylelint-scss helps
The plugin/no-browser-hacks raises false positives for scss variables, like $my-variable

GuilhermeHideki commented 4 years ago

Files:

[x] ./src/assets/css/clearbutton.css (#1236)
[x] ./src/assets/css/detailtable.css (#1236)
[x] ./src/assets/css/flexstyles.css (#1236)
[x] ./src/assets/css/fonts.css (#1236)
[x] ./src/assets/css/fonts.sized.css (#1236)
[x] ./src/assets/css/ios.css (#1236)
[x] ./src/assets/css/livetv.css (#1236)
[x] ./src/assets/css/scrollstyles.css (#1236)
[x] ./src/assets/css/site.css (#1236)

heyhippari commented 4 years ago

We currently have a large number of media queries with different breakpoints. From a quick look in the source they are all over the place (32em, 50em, 84em, 70em, 34.375em, 31.25em, 720px, 1000px, 37.5em, 68.75em, and many, many more).

The logic of these is a bit lost on me, so I propose the following: adopt Bootstrap's breakpoints.

They have the following:

Phone (portrait): 0
Phone (landscape): 576px
Tablets: 768px
Desktop: 992px
Large Desktop: 1200px
Very Large Desktop: 1400px

This gives us a rational set of breakpoints, while having a familiar (and limited) set that is not confusing for contributors and is battle-tested by what is likely the largest CSS framework out there.

thornbill commented 3 years ago

Everything except for themes has been converted to Sass now. Themes will require some additional webpack configuration to be individual outuputs since they are loaded in dynamically at run time.

jellyfin / jellyfin-web

Changing stylesheets to SCSS #836