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

Declare & Use CSS Custom Properties #3146

Merged
merged 20 commits into from
Nov 4, 2021
Merged

Declare & Use CSS Custom Properties #3146

merged 20 commits into from
Nov 4, 2021

Conversation

SychO9
Copy link
Member

@SychO9 SychO9 commented Nov 2, 2021

Fixes #2600

Changes proposed in this pull request:

  • Declare CSS custom properties for Less variables where possible.
  • Use CSS custom properties instead of Less variables where possible.
  • Declare color variations as CSS custom properties before using relevant mixins.
  • Make some hard-coded colors into CSS custom properties.

Reviewers should focus on:
Most of the changes are simple replacements of the common variables. What you should pay attention to is the following:

  • root.css declaration of custom properties.
  • Backwards compatibility.
  • Anything that I might've missed about this.
  • Mixins and the way they work (.Button--color(), .light-contents())
    For example the Button--color mixin used to be directly called as such:
.Button {
    .Button--color(@control-color, @control-bg);
}

.Button--primary {
    .Button--color(@body-bg, @primary-color);
}

Now the mixin is called like this:

:root {
    .Button--color-vars(@control-color, @control-bg, 'button');
    .Button--color-vars(@body-bg, @primary-color, 'button-primary');
}

.Button {
    .Button--color-auto('button');
}

.Button--primary {
    .Button--color-auto('button-primary');
}

This is done so that the color variations created by the mixin are declared as CSS custom properties first, as it would make it possible for us to switch to a darkmode with a simple class name change.

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

Required changes:

  • Related documentation PR: (Remove if irrelevant): Some things here should be documented for theme designers and extension devs.

less/admin/PermissionsPage.less Show resolved Hide resolved
less/common/App.less Outdated Show resolved Hide resolved
less/admin/UsersListPage.less Outdated Show resolved Hide resolved
less/common/mixins/light-contents.less Show resolved Hide resolved
less/common/App.less Outdated Show resolved Hide resolved
less/common/Button.less Show resolved Hide resolved
less/common/Checkbox.less Show resolved Hide resolved
less/common/Modal.less Show resolved Hide resolved
less/common/mixins/button-color.less Show resolved Hide resolved
less/forum/DiscussionPage.less Outdated Show resolved Hide resolved
@askvortsov1
Copy link
Member

This is huge! Really excited to see us taking this big step towards themability and modern architecture.

Copy link
Member

@davwheat davwheat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Just one minor suggestion.

less/common/App.less Outdated Show resolved Hide resolved
Co-authored-by: David Wheatley <hi@davwheat.dev>
Copy link
Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent!

@SychO9 SychO9 merged commit 809df29 into master Nov 4, 2021
@SychO9 SychO9 deleted the sm/css-custom-properties branch November 4, 2021 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CSS Variables
3 participants