Skip to content
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

Split CSS file into smaller ones #5120

Open
winniehell opened this issue Aug 11, 2019 · 11 comments
Open

Split CSS file into smaller ones #5120

winniehell opened this issue Aug 11, 2019 · 11 comments

Comments

@winniehell
Copy link
Contributor

The current stylesheet is almost 1000 lines long. That is quite a lot. I'd like to suggest to split it into smaller files by the top level widget the styles are affecting.

For example

QTreeView {
	outline: none;
}

QTreeWidget::item:hover,
QTreeWidget::branch:hover {
	background-color: #3C444E;
}

would go into a QTreeWidget.css.

Some styles that affect a group of unrelated widget may still end up in a special global.css such as these lines:

QLabel, QTreeWidget, QListWidget, QGroupBox, QMenuBar {
	color: #d1d8e4;
}

Hopefully these will relatively few though.


The advantages of splitting into smaller files would be:

  • merge conflicts are less likely to happen
  • styles for a certain component are easier to find (AutomationEditor.cpp --> AutomationEditor.css)

In the future this may also allow to inherit from the default theme and only override the styles for some components.


The technical side of this is fairly simple: Because the Qt Resource System allows to list files in the resource directories, we can simply load all *.css files.

@winniehell
Copy link
Contributor Author

@lukas-w Given that you introduced the Qt Resource System (#1891), I'd like to here your opinion on this.

@lukas-w
Copy link
Member

lukas-w commented Aug 12, 2019

As long as it can be split into logical subsets, why not. Plugin-specific styles such as the WatsynView can certainly be moved out of the main .css or even be moved to their respective plugin folder. One thing to keep in mind is that the order the *.css files are read will matter because rules can overwrite each other if they have the same specificity.

Spreading the stylesheet across too many files as in the proposed "one widget per file" (QTreeWidget.css, AutomationEditor.css) could defeat the purpose though. Maybe a coarser split is easier to navigate, for instance moving out the plugins + something like main.css and editors.css.

@lukas-w
Copy link
Member

lukas-w commented Aug 12, 2019

Sorry, hit "Comment" accidentally.

The exact way the stylesheet is split is probably a matter of taste, so feel free to do it the way you think is intuitive. I'd just prefer it if we didn't end up with 50+ micro-files.

Side note, there may be other ways to simplify the stylesheet, for instance the text color #d1d8e4 appears 16 times throughout the stylesheet even though it's already part of the palette, which I believe should make it propagate throughout the application.

@winniehell
Copy link
Contributor Author

@lukas-w Thank you for your thoughts! 🙇‍♂️

One thing to keep in mind is that the order the *.css files are read will matter because rules can overwrite each other if they have the same specificity.

I agree that this can become a problem. The same problem can occur when moving blocks around in one big file. In my opinion the solution would be to make rules as specific as they need to be to take precedence.

Spreading the stylesheet across too many files as in the proposed "one widget per file" (QTreeWidget.css, AutomationEditor.css) could defeat the purpose though.

My main goal behind this suggestion was to have a simple rule behind the split. Even a linter for

All rules in AutomationEditor.css need to begin with AutomationEditor

would be easy to write.

Defining "logic units" to do the split can be highly subjective (for example buttons.css vs. song-editor.css) and is definitely more difficult to lint.

@Veratil
Copy link
Contributor

Veratil commented Aug 12, 2019

Could this split benefit from using something like SCSS? I think it would allow writing themes to become a lot easier (if that's something someone wants to do).

@winniehell
Copy link
Contributor Author

Could this split benefit from using something like SCSS?

@Veratil I think SCSS can help reduce the overall size of the CSS. However I would make introducing it a separate step from the split because it adds some complexity to the toolchain.

@Veratil
Copy link
Contributor

Veratil commented Aug 12, 2019

Could this split benefit from using something like SCSS?

@Veratil I think SCSS can help reduce the overall size of the CSS. However I would make introducing it a separate step from the split because it adds some complexity to the toolchain.

Agreed, the pros and cons must be weighed. The end result stylesheet might be the same size, but the ability to theme would become much easier (which I think could really open up customizing workflows to users needs). Introducing a new step in the toolchain should be a one-time thing, but it's adding a new dependency as well.

@SecondFlight
Copy link
Member

Isn't the CSS loaded dynamically? I'm worried allowing SCSS would require adding a dedicated SCSS compiler to LMMS, not just to the build chain.

@winniehell
Copy link
Contributor Author

winniehell commented Aug 12, 2019

I'm worried allowing SCSS would require adding a dedicated SCSS compiler to LMMS, not just to the build chain.

@SecondFlight You could compile the SCSS to CSS at build time and then check in the resulting CSS before distribution. This would then leave the loading mechanism untouched.

However if we agree that introducing SCSS is not necessarily part of splitting the CSS, maybe we can continue discussion in a separate issue and keep this one focussed on the split?

@SecondFlight
Copy link
Member

SecondFlight commented Aug 12, 2019

You could compile the SCSS to CSS at build time and then check in the resulting CSS before distribution.

True, though that doesn't account for custom themes. Custom themes would still need to use CSS, meaning the process for editing the default theme in the repo would be different from the process for creating a new custom theme.

Not a disqualifier by itself I suppose, but worth taking into consideration.

@musikBear
Copy link

👍 for separate CSS-files for specific components. That will make finding specific items simpler, and simpler is good.
Accessibility and intuitive code always have my support ;p
Not so sure about SCSS, mostly for the same reason. If the complexity goes up, where it could be kept simple, it exclude users

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants