Skip to content

Conversation

@CarlSchwan
Copy link
Member

@CarlSchwan CarlSchwan commented May 23, 2022

@CarlSchwan CarlSchwan added the 3. to review Waiting for reviews label May 23, 2022
@CarlSchwan CarlSchwan self-assigned this May 23, 2022
@CarlSchwan CarlSchwan force-pushed the cleanup/remove-scssphp branch from f1de9cb to 8e99013 Compare May 23, 2022 07:37
@nickvergessen
Copy link
Member

Pr is merged, now rebase here?

@CarlSchwan CarlSchwan force-pushed the cleanup/remove-scssphp branch from 8e99013 to ad011b2 Compare May 23, 2022 07:59
@nickvergessen nickvergessen added this to the Nextcloud 25 milestone May 23, 2022
@CarlSchwan
Copy link
Member Author

CarlSchwan commented May 23, 2022

this breaks the dyslexia font, I'm working on a fix

I'm now prefixing the custom CSS manually now.

@CarlSchwan CarlSchwan added 2. developing Work in progress and removed 3. to review Waiting for reviews labels May 23, 2022
Not used aymore

Signed-off-by: Carl Schwan <[email protected]>
@CarlSchwan CarlSchwan force-pushed the cleanup/remove-scssphp branch from ad011b2 to e307f8b Compare May 23, 2022 09:14
@CarlSchwan CarlSchwan added 3. to review Waiting for reviews and removed 2. developing Work in progress labels May 23, 2022
@CarlSchwan CarlSchwan requested a review from szaimen May 24, 2022 09:15
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

$compiler = new Compiler();
$compiledCss = $compiler->compileString("body[data-theme-$themeId] { $variables $customCss }");
$css = $compiledCss->getCss();;

@CarlSchwan
Copy link
Member Author

$compiler = new Compiler();
$compiledCss = $compiler->compileString("body[data-theme-$themeId] { $variables $customCss }");
$css = $compiledCss->getCss();;

See this PR this removes the usage of it and do the simple CSS prefixing directly.

@skjnldsv
Copy link
Member

See this PR this removes the usage of it and do the simple CSS prefixing directly.

I tested the perf of the scsscacher and it seemed more than ok.
I think this is much more complex and difficult to read. Also it now means themes can inject code that doesn't impact their theme only. I prefer a safe scope :)

@CarlSchwan
Copy link
Member Author

See this PR this removes the usage of it and do the simple CSS prefixing directly.

I tested the perf of the scsscacher and it seemed more than ok. I think this is much more complex and difficult to read. Also it now means themes can inject code that doesn't impact their theme only. I prefer a safe scope :)

This is a 15 lines patch that would allow us to remove 19 000 lines of code. I still think it is worth it, even if the performance reasons are really minor. This is less work in the long term for handling the dependency bump (we are currently minor 2 versions in master and 6 in stable22) and limit a bit more the risk of supply chain attacks.

I'm actually even surprised that @font-face works currently since @font-face can't be nested inside a CSS query and it seems that scssphp will unnest it differently from that node-sass will do. And I don't like depending on nonstandard behavior.

@skjnldsv
Copy link
Member

And I don't like depending on nonstandard behavior.

With scss it's proper.

This is a 15 lines patch that would allow us to remove 19 000 lines of code

This is very different since we're talking about a dependency :)
I'm talking about our code maintainability. I don't think the approach this PR do is acceptable 🤷

@CarlSchwan CarlSchwan closed this May 27, 2022
@skjnldsv skjnldsv deleted the cleanup/remove-scssphp branch March 14, 2024 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants