LPS-115484 As a developer I can declare theme's customizable CSS variables #17
Conversation
|
To conserve resources, the PR Tester does not automatically run for every pull. If your code changes were already tested in another pull, reference that pull in this pull so the test results can be analyzed. If your pull was never tested, comment "ci:test" to run the PR Tester for this pull. |
|
ci:test:sf |
|
ci:test |
liferay-continuous-integration
added
ci:test:sf - pending
ci:test:sf - success
and removed
ci:test:sf - pending
labels
✔️ ci:test:sf - 1 out of 1 jobs passed in 3 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: LPS-115484 1 Successful Jobs:For more details click here. |
liferay-continuous-integration
added
ci:test - pending
ci:test - failure
and removed
ci:test - pending
labels
|
Hey @izaera, do we need LPS-115485 commits in here? If we do, we'll need to wait for those to get merged to safely push this... if they aren't, could you take them out so we can work on this independently? |
...d/css/variables/web/internal/theme/ThemeCSSVariableDescriptionsServiceTrackerCustomizer.java
Outdated
Show resolved
Hide resolved
...d/css/variables/web/internal/theme/ThemeCSSVariableDescriptionsServiceTrackerCustomizer.java
Outdated
Show resolved
Hide resolved
.../src/test/resources/com/liferay/frontend/css/variables/web/internal/theme/css-variables.json
Outdated
Show resolved
Hide resolved
.../src/test/resources/com/liferay/frontend/css/variables/web/internal/theme/css-variables.json
Outdated
Show resolved
Hide resolved
Yes, but only because they create the project folders with the gradle and bnd files. But other than that, nothing else is shared, so we may work on this PR independently from the other (that's why I made two PRs in first place). It's only that we cannot forward this until the other gets merged which, on the other hand, wouldn't make too much sense. |
…or a theme The ThemeCSSVariableDescriptionsRegistryImpl class obtains CSS variables from a /WEB-INF/css-variables.json file present inside the theme's WAR file.
We will use field liferayTheme.cssVariablesPath to store that value, and 'css-variables.json' will be the default value in case it is missing.
This is to avoid race conditions in case someone accesses the map in the middle of an update.
Don't support localization in the service itself for now. We will see in the future how we model it, but we don't want to couple it with the service itself, since it is a very specific UI level thing.
First, we will use the Tokens-Path bundle header to get the path to the JSON file inside the WAR and will use WEB-INF/tokens.json as default value. Then we will change the format of the JSON file to make it more extensible and maintainable in the future. Note that themes are WAR files, not OSGi bundles, so people need to add the Tokens-Path header to the liferay-plugin-package.properties file to make it appear in the MANIFEST.MF file of the generated OSGi bundle. Also note that we check for the existence of a </theme> tag in the liferay-look-and-feel.xml file to distinguish theme WAR files from other type of WAR artifacts.
|
@jbalsas I have force-pushed (because I needed to rebase) the requested changes. Do you mind having a look in case something else needs to be polished? Thx. The real changes start at commit 2066d40 (i.e.: only the last three are new) with the last one being the more important. The rest remain the same, as in the previous PR. I have implemented the change from |
|
ci:test:sf |
| */ | ||
| public interface CSSVariableDescription { | ||
|
|
||
| public CSSVariableType getCSSVariableType(); |
There was a problem hiding this comment.
Hey @izaera, thanks for the changes! Seeing it it all starts to make sense in my mind...
The more I think about it the more I like the "Tokens" idea. It makes it all a bit more generic and flexible and it's actually easier to explain than CSSVariables.
Now that I think about it... since we've already merged this, chances are we won't be able to change it if Brian publishes, so maybe the first thing to do would be to add a .releng-ignore thingie to make sure we have time until the next release to flesh this out.
I'm trying to wrap up these changes with the ones we already pushed to master... sorry, getting hard to keep track of everything :(
I'd say we need something that's called css-variables which is the thing we definitely use to print the CSS variables.
That system should be fed with tokens or something like that and know how to transform them... that would allow us to add additional tokenTransformer approaches when needed and decouple implementation from concept.
Does this make sense?
I split it up into two pulls for that reason ;-). The first PR is I would say this is CSS variables too because it only adds a service to discover which CSS variables a theme supports. And we know that they are CSS variables because it's a theme. However, I see place for tokens in a theme (for example inside templates, or predefined content) so I'm not sure what is best:
The only thing that scares about 2 is that tokens may be different things covered under the same abstraction in the future. For example: CSS variables now have a name and type. One of the types is To sum up: I'm OK with naming this tokens if we at least have a clear idea of what other tokens may look like (i.e: if we can define what is a token and what isn't now, in advance). |
|
Putting this on hold momentarily since TokenDefinition is in a bit of a definition phase itself :) |
|
Pinging @ealonso, @ruben-pulido and @JorgeFerrer here so we can start this again. This is an initial PR we had before the StyleEditor epic started moving that does a bunch of things that might not be necessary. While we discuss the JSON format I'm looking for input as to what to do in regards:
|
|
The name of the file must reflect as accurately as possible what it is, not what it's used for. Since this is a definition of what the available tokens are, I believe that token-definition.json is a good name. Thoughts? I don't have a clear opinion on the JSON vs Java question right now, so I'd prefer other people to chime in on that one. |
|
Hey @izaera, I'm going to summarize my understanding of what we have and what I think we could build next to try to unblock this. Please, correct me as needed! 🤗 frontend-css-variables modulesWe currently have 2 theme token definitionOur next step would be to provide an interface to:
Waiting on @ealonso and @ruben-pulido to answer #17 (comment), I think we could simply return the Based on this, I'd suggest:
@JorgeFerrer, does this seem aligned with Echo's roadmap? (can't ping Tarik since he still doesn't have a GitHub alias... but sending him a message on Slack) @ealonso, @ruben-pulido, do you think this should be enough to implement the StyleBook creation or would you require something else? |
A String should be enough at this moment.
What do you mean with, this should be enough? |
So, once we do this we'll have:
Do you think there's any additional Infrastructure or Component you'd like us to provide or work on? For example, we discussed having an autogenerated UI which was discarded. Another thing that we could explore is providing a unified way of "previewing" changes. This is mostly tied to the UI, and likely some simple JS based on our past experience, but we can flesh it out more if needed. |
LGTM :-) |
If a token is really a CSS variable, I would call it CSS variable, since that way you don't need two terms. However, I think token is higher level than CSS variable, since you may have an UI to define tokens that will be injected as CSS variables in themes, but could be injected as some other thing (for example, an inline style) in other place of the portal. Theoretically you could even use a color token to colorize images/logos. Said that, the JSON file that goes inside a theme file, AFAIK, can only be considered as a description of CSS variables as they will only be used inside CSS rules (that is: the JSON file describes low level CSS variables, not high level UI tokens). Unless we plan to use them in templates, or other content inside theme bundles... |
That JSON must have a way to map tokens with CSSVariables, but in no way is a description of CSS Variables. Now that you mention it... one way to simplify what we're discussing over at https://github.com/jbalsas/liferay-portal/pull/2218 would be to split into 2 files... |
But wouldn't that be a 1-1 mapping? To elaborate more my view: my assumptions are that a theme bundle is deployed and, because it is a theme, it defines the CSS variables it lets override. Then, some OSGi bundle in the portal reads those CSS variable definitions, interprets them as tokens, and shows an UI to the user to define those tokens (and possibly some tokens coming from other non-theme places). Then, the OSGi bundle that showed the UI to define those tokens takes care of registering a ScopedCSSVariablesProvider to inject the values for the theme (and possibly other services to render the non-CSS tokens). Does that makes sense? I mean, why letting the theme know anything about tokens? What is the advantage? |
It kinda does, yeah... but I think it's limiting in what we want to do for the future
I think @JorgeFerrer has a pyramid with this and what we want to accomplish 😂 . If you check Tarik's presentation, it was outlined there, and some things we'd need for the future might or might not be implemented through CSS Variables. I think it's rather unfortunate that right now this is our only possible way forward. We shouldn't bank all our strategy (naming, APIs, roadmap) in this, though. |
That's why I was proposing to call it CSS variables (only inside themes) which is the only thing we have clear right now ;-). However, given that we are only going to return a JSON string I will call it tokens for now and we'll see what happens in the future 😅 |
I think it is good enough for the moment, once we start working with it, we can give you more feedback |
Great, thanks!! @izaera, going to close this one since the solution will be pretty different... please link to this when you open a new one? 🙏 |
|
This is hopefully the final PR -> brianchandotcom#91176 Linking for the record. |
This was sent to me at https://github.com/wincent/liferay-portal/pull/357 — tests failed there, so I am going to take that as an opportunity to relocate this here instead and re-run them. Note the dependency on a prior pull (https://github.com/wincent/liferay-portal/pull/354) — the last three commits are the new ones, based on top of the other pull.
cc @izaera
Original message follows.
This is the continuation of https://github.com/wincent/liferay-portal/pull/354.
This is a partial PR based on the original PoC https://github.com/jbalsas/liferay-portal/pull/2211. It adds a service to retrieve CSS variable declarations inside themes' WAR files. These declarations will then be used to create customization forms in future PRs. Then, those forms will save configuration values that will be retrieved by a
ScopedCSSVariablesProviderto inject them in the rendered HTML.There's nothing to test manually in this PR as it is not yet possible to do anything from the GUI.