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

XWIKI-22667: Introduce CSS equivalents for all LESS variables #3936

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

Sereza7
Copy link
Contributor

@Sereza7 Sereza7 commented Feb 26, 2025

Jira URL

https://jira.xwiki.org/browse/XWIKI-22667

Changes

Description

  • Added a bunch of CSS variables to make it possible to move some stylesheets from LESS to CSS. In this first pack, there's about 120 variables.
  • In priority, I wanted to make sure anything set in the color themes was converted nicely.

Clarifications

  • This is the first step of a proposal described on https://design.xwiki.org/xwiki/bin/view/Proposal/MovingawayfromLESSCSS
  • Those CSS variables are not API content yet (explained it in the template comments), there'll probably be a lot of changes. However, this is a good base and it's quite straightforward to extend or reduce it.
  • This is code we don't use actively yet. I can't be bothered to source this statement, but AFAIR we should not add code to XS that's not useful (avoid the mindset "this might be useful one day" that ends up in dead code cluttering the codebase). However it's been discussed and there'll definitely be a lot of uses very soon. IMO it's alright to add it by itself.

Screenshots & Video

No UI change yet!
We're only making things available in CSS:
image
Note that the generated CSS doesn't look very nice, but it compiles and the source template is legible so it's alright IMO.
image
All variables needed for Iceberg are here. Some lines are in grey, this is because this variable value is overridden by another line later. This is not the most efficient, but it doesn't cost much (browsers optimize this kind of things on their side, it seems Firefox just straight up just disables the lines) and it's a good safety against bugs. E.g. for one reason or another, we decide to remove --list-group-link-hover-color , but we don't update the value of --component-active-color in the Iceberg colorTheme. We still have a base value for --component-active-color instead of nothing at all.

Executed Tests

Manual tests with Iceberg. Tried out the font-size functions, they work properly (needed a hack and a few tries before that worked, it's some pretty advanced CSS operations ^^'). Tried out a couple colors, they work as expected.
The Firefox devtool highlight all the variables in formulas correctly, this means they are actually working and not just string values in the hiding.

I did not test all the other colorThemes, the current changes do not break them BUT they will be way more complicated to migrate than Iceberg. Iceberg migration is pretty much done with those changes already. Bootswatch colorThemes for example have massive LESS sheets in their @lessCode parameter. Those sheets still need to be migrated/duplicated to apply their changes to CSS variables, it doesn't just work from scratch with what I've put on here.

I didn't write any automatic test yet, I'd rather check in some Flamingo test once I updated a Flamingo stylesheet to use CSS variables. Not sure how we'd check CSS variables in the Java test suite anyways.

Expected merging strategy

  • Prefers squash: Yes
  • Backport on branches:
    • None, this is a rather large change, will be followed by large changes and should only be on 17.X

Points to clarify before a merge

  • Are the name and type of the file correct? I tried ending it in .css.vm but it wouldn't work as a stylesheet link. Right now the code completion and linting are horrid but I'm not sure how we could improve that.
  • Do we agree on the priority of the variable sets? XSspecific>Bootstrap>oldcolortheme

Sereza7 and others added 16 commits February 26, 2025 14:26
* Replaced any LESS variable value with their CSS counterpart (from LESS inheritance to CSS inheritance system).
* Added all the LESS variables defined in XWiki Standard to the CSS mapper.
* TODO: see how to get the actual values of the variables in CSS.
* WIP, fixed the values for xsVariables and added a couple variables from bootstrap.
* Added fallbacks for all colorTheme variables
* Added support for a lot of oldColorTheme variables (velocity ones)
* Updated codestyle
* Added a couple missing variables from the default values used in Iceberg.
* Updated codestyle
* Added a couple missing variables from the default values used in Iceberg.
"breadcrumb-bg": "color",
"breadcrumb-color": "color",
"breadcrumb-link-color": "color",
"breadcrumb-separator": "escapedText",
Copy link
Contributor

Choose a reason for hiding this comment

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

I see escapedText being used later in the code. But does the other values have associated semantics?

Copy link
Contributor Author

@Sereza7 Sereza7 Mar 3, 2025

Choose a reason for hiding this comment

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

I didn't use the other values yet. Down the line we can use the @property annotation, but for now I just went with the "easy" one line instantiation.

https://developer.mozilla.org/en-US/docs/Web/CSS/@property

"100vw": "100vw",
"int-viewport-width": "calc(10000 * tan(atan2(var(--100vw), 10000px)))",
"font-size-document-title": "calc(max(var(--min-document-title-font-size) * 1px, min(var(--max-document-title-font-size) * 1px, var(--min-document-title-font-size) * 1px + (var(--max-document-title-font-size) - var(--min-document-title-font-size)) * (var(--int-viewport-width) - var(--min-screen-size)) / (var(--max-screen-size) - var(--min-screen-size)))))",
"font-size-h1": "calc(max(var(--min-h1-font-size), min(var(--max-h1-font-size), var(--min-h1-font-size) + (var(--max-h1-font-size) - var(--min-h1-font-size)) * (var(--int-viewport-width) - var(--min-screen-size)) / (var(--max-screen-size) - var(--min-screen-size)))))",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not fan of seeing

@font-size-h1: ~"max(@{min-h1-font-size}px, min(@{max-h1-font-size}px, calc(@{min-h1-font-size}px + (@{max-h1-font-size} - @{min-h1-font-size}) * (100vw - @{min-screen-size}px) / (@{max-screen-size} - @{min-screen-size}))))";
being duplicated.

Copy link
Contributor Author

@Sereza7 Sereza7 Mar 3, 2025

Choose a reason for hiding this comment

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

I'm not sure we can factorize this with the current CSS. I'll look a bit more for a solution. AFAIK, this would have been addressed with a mixin with parameters in LESS, not sure if there's a way to make it look cleaner in native CSS right now.


I wasn't sure what was the exact intended result for the calculation of this variable, so I made my best to just replicate its formula in CSS. This is also a good exercise/PoC about CSS calc: it's possible to move pretty much the whole formula, but some details (esp. units) will make implementation a bit more difficult.


What do you think should be done with those variables instead of a 1to1 port to CSS variables?
Should we remove them from our CSS variable-set and not consider them API ?
Did you mean that we should remove the LESS alternative ASAP?

PS: CSS variables are evaluated lazily (opposite to LESS variables that are evaluated on compilation to CSS). It's barely any cost to load CSS variables that are not used.

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.

3 participants