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

feat: add Less custom function extender, is-extension-enabled function #3190

Merged
merged 40 commits into from
Dec 14, 2021

Conversation

davwheat
Copy link
Member

@davwheat davwheat commented Dec 9, 2021

Changes proposed in this pull request:

  • create a new custom less function function on our Theme extender
  • creates a new Less function for applying styles only when a specific extension is enabled

This prevents stylesheet bloats from extensions which extend multiple extensions, by only adding styles which are needed.

Extender usage:

    (new Extend\Theme())
        ->addCustomLessFunction('test', function () {
            return new \Less_Tree_Quoted('', 'true');
        })

Is ext enabled func usage:

// Is extension enabled function:
body when (is-extension-enabled('flarum-tags') = true) {
  // enabled
}
body when (is-extension-enabled('flarum-tags') = false) {
  // not enabled
}

Reviewers should focus on:

(no longer applicable)
Would a simpler syntax be better, such as:

.if-extension-enabled(flarum-tags, {
  // ...
});

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)
  • Related core extension PRs: (Remove if irrelevant)

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.

Would a simpler syntax be better, such as:

I think so. What's the benefit of doing it the other way?

less/common/mixins/extension-styles.less Outdated Show resolved Hide resolved
@davwheat
Copy link
Member Author

davwheat commented Dec 9, 2021

Would a simpler syntax be better, such as:

I think so. What's the benefit of doing it the other way?

There's no inherent benefit of either way, I don't believe. Just syntactically different.

@davwheat
Copy link
Member Author

davwheat commented Dec 9, 2021

So I've pushed an alternate syntax which I think may be better.

The original syntax required that the mixin be declared in the root scope, meaning that

// This would work
.if-extension-enabled(@ext) when (@ext = flarum-tags) {
  body { background: black; }
}

// ...but this would NOT
body {
  .if-extension-enabled(@ext) when (@ext = flarum-tags) {
    background: black;
  }
}

The new syntax, however, does support this:

// This would work
.when-extension-enabled(flarum-tags, {
  body { background: black; }
});

// ...and so does this
body {
  .when-extension-enabled(flarum-tags, {
    background: black;
  });
}

However, the newer method will be significantly less performant, since it performs a loop over all enabled extensions each time it is called, rather than only doing it once.

We could reduce that by setting a less variable per enabled extension (e.g. @extension-enabled--flarum-tags), then use Less's variable-variables support to check for these, but that seems a bit hacky, imo.

@SychO9
Copy link
Member

SychO9 commented Dec 11, 2021

What worries me about this is that, even without this, the Less compiler breaks the forum sometimes by running out of memory, it has been reported multiple times before, I've even experienced it locally, so I'm personally very reluctant to introducing anything that could reduce its performance even less.

How about registering a custom function instead ? I haven't actually tested the difference (yet) but I'm assuming it would be a more efficient solution. (tested locally and it works).

.App {
  & when(is-extension-enabled('flarum-tags') = true) {
    opacity: 0.3;
  }
}
$parser->registerFunction('is-extension-enabled', function (Less_Tree_Quoted $extensionId) {
    return new Less_Tree_Quoted('', in_array($extensionId->value, ['flarum-tags', 'flarum-blog']) ? 'true' : 'false');
});

@davwheat
Copy link
Member Author

That does seem like a much better idea.

When I saw the documentation of less-php state that custom functions weren't supported, I assumed it meant completely, but if we can define them in PHP, that's perfect!

@davwheat davwheat changed the title feat: add Less mixin to apply styles only when a specific extension is enabled feat: add Less custom function extender, is-extension-enabled function Dec 12, 2021
@davwheat
Copy link
Member Author

@SychO9 Should be done now :)

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.

Overall makes sense, but is the = true / = false really necessary? Is there no "not" symbol

src/Frontend/FrontendServiceProvider.php Outdated Show resolved Hide resolved
src/Extend/Theme.php Outdated Show resolved Hide resolved
src/Extend/Theme.php Show resolved Hide resolved
src/Extend/Theme.php Outdated Show resolved Hide resolved
src/Frontend/Assets.php Outdated Show resolved Hide resolved
src/Frontend/Assets.php Outdated Show resolved Hide resolved
src/Extend/Theme.php Outdated Show resolved Hide resolved
Copy link
Member

@SychO9 SychO9 left a comment

Choose a reason for hiding this comment

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

LGTM!

just nitpicks, because they're fun :p

src/Extend/Theme.php Outdated Show resolved Hide resolved
src/Extend/Theme.php Outdated Show resolved Hide resolved
src/Extend/Theme.php Outdated Show resolved Hide resolved
Co-authored-by: Sami Mazouz <sychocouldy@gmail.com>
src/Extend/Theme.php Outdated Show resolved Hide resolved
@davwheat davwheat merged commit 11fd012 into master Dec 14, 2021
@davwheat davwheat deleted the dw/less-extension-styles branch December 14, 2021 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants