NPM Resource Changed ? Pending

User tests: Successful: Unsuccessful:

avatar laoneo
laoneo
24 Jun 2021

Pull Request for Issue #34559.

Summary of Changes

#33100 introduced atum specific CSS variables across the core. This shouldn't be the case as extensions should be independent of the template. It would be better to have these variables with a generic name so other templates can provide them as well.

@ciar4n and @dgrammatiko any feedback?

Testing Instructions

Browse the core, especially the media manager.

Actual result BEFORE applying this Pull Request

All looks the same.

Expected result AFTER applying this Pull Request

All looks the same.

Documentation Changes Required

It needs to be documented which variables a template has to provide.

avatar laoneo laoneo - open - 24 Jun 2021
avatar laoneo laoneo - change - 24 Jun 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 24 Jun 2021
Category Administration Templates (admin) NPM Change
avatar brianteeman
brianteeman - comment - 24 Jun 2021

Same comment as in the other pr #34624 (comment)

avatar laoneo
laoneo - comment - 24 Jun 2021

Please comment here @brianteeman & @dgrammatiko as this is the pr to decide about the variable setup andbia more important. #34624 is just a followup... It will be easier for others to fillow the decission path.

@wilsonge I would mark this one here as release blocker as it basically sets the path for other templates.

avatar brianteeman
brianteeman - comment - 24 Jun 2021

Sorry I saw this one second - will comment here in future

It's fine as inline styles, but this needs to be documented that AFFECTS every front end template.

Doing it in a stylesheet will mean that it can be updated in a new release without having to ask every template developer and site owner to update their own templates.

So wouldn't this be more obvious (and useful) as every template will need it

Pseudo code untested

// Enable assets
$wa->usePreset('template.cassiopeia.' . ($this->direction === 'rtl' ? 'rtl' : 'ltr'))
	->useStyle('template.active.language')
	->useStyle('template.user')
	->useScript('template.user')
	// global design classes
	->useStyle('template.global');
avatar dgrammatiko
dgrammatiko - comment - 24 Jun 2021

My comment on the other PR:

It's fine as inline styles, but this needs to be documented that AFFECTS every front end template.
Also, a personal preference would be to change the prefix from template to jdt (joomla design token) or something similar. The template prefix is not really representative that these are Global design tokens. But maybe that's just me.

Doing it in a stylesheet will mean that it can be updated in a new release without having to ask every template developer and site owner to update their own templates.

You could go that way but since these should be template dependant nobody will ever use that css file. So, the best solution is documenting and communicating (eg add a multiline comment in the templates/index.php at the very top stating that COMPATIBLE JOOMLA TEMPLATES SHOULD DEFINE THESE CSS VARS )

avatar brianteeman
brianteeman - comment - 24 Jun 2021

But then if we ever need to change the vars (or add new ones) we can't

avatar laoneo
laoneo - comment - 24 Jun 2021

That's always the case with an API. And this is what we basically define here. During the lifecycle of version 4,we can add new ones but not rename or delete. Adding needs to come with a proper default.

avatar brianteeman
brianteeman - comment - 24 Jun 2021

But you can't add new ones your way and you can do my way

avatar laoneo
laoneo - comment - 25 Jun 2021

Not sure what you mean. Adding is never an issue when you define a proper default.

avatar brianteeman
brianteeman - comment - 25 Jun 2021

Let me try again.

Scenario 1 (the current proposal for inline styles)
all template builders copy the 6 lines into their own template
Joomla 4.0.1 is released with a bug fix to one of those lines (or adding a new line)
all template builders have to update their own template manually

Scenario 2 (using a template.global css file)
all template builders copy the 1 line into their own template
Joomla 4.0.1 is released with a bug fix to one of those lines (or adding a new line)
all template builders have to do nothing at all

avatar laoneo
laoneo - comment - 25 Jun 2021

This is only the case when the template includes the css file. But honestly I do not care if they are inline or if they are in a file. I would even prefer to have them in a file, so it would be then much clearer.

avatar brianteeman
brianteeman - comment - 25 Jun 2021

So please put them in a file and we can both be happy

avatar laoneo
laoneo - comment - 25 Jun 2021

How do you want to pass the colors which are coming as params into the the file then?

avatar brianteeman
brianteeman - comment - 25 Jun 2021

Sorry - I am only talking about the non-atum template which doesnt have params

avatar dgrammatiko
dgrammatiko - comment - 25 Jun 2021

@brianteeman your idea is flawed because the file can only have predefined colours which 99% will not match the template colours. So, one way or another the template builders will have to override the CSS Vars values to match their templates. Also the documentation should be a static site: design-system.joomla.org which defines the design system of Joomla sites similar to https://make.wordpress.org/design/handbook/design-guide/

avatar brianteeman
brianteeman - comment - 25 Jun 2021

These colours are only used in site admin tools like media manager so nothing to match

Think like a user not a designer or developer.

avatar dgrammatiko
dgrammatiko - comment - 25 Jun 2021

Think like a user not a designer or developer.

Users have nothing to do with this as it only affects the template builders...

avatar brianteeman
brianteeman - comment - 25 Jun 2021

cough, cough, cough, splutter, splutter.

Not every site purchases a template from a store, club or developer.

avatar laoneo
laoneo - comment - 25 Jun 2021

I will have a look next week and come up with a proposal.

avatar Fedik
Fedik - comment - 28 Jun 2021

I think it is good as it is.

But If need as a file.
Then can create one, and add global asset,

    {
      "name": "template.global-variables",
      "type": "style",
      "uri": "global-variables.css",
      "description": "Joomla default CSS variables ... blabla description"
    },

somewhere after here:

{
"name": "template.active",
"type": "style",
"uri": "",
"description": "A dummy asset to allow to extensions to use it as dependency to active template"
},

Then add it as dependency in each template.
A templates can override this file by placing it under <template>/css/global-variables.css (or override whole asset).

However, this means that we need to update the file after User change the template parameters.
That is a hard part here :)

But for a templates that does not have such parameters it should work good, User even able to edit this file in a template editing page.

avatar brianteeman
brianteeman - comment - 28 Jun 2021

However, this means that we need to update the file after User change the template parameters.

I am only saying to move it to a file for frontend templates where there are no paramaters

avatar Fedik
Fedik - comment - 28 Jun 2021

That will work, I wrote it in last sentence ;)

avatar laoneo laoneo - change - 29 Jun 2021
Labels Added: NPM Resource Changed ?
avatar laoneo
laoneo - comment - 29 Jun 2021

@Fedik can you help me out here with asset dependencies. Getting always the following error error on this commit:
Unsatisfied dependency "template.global-variables" for an asset "template.cassiopeia.ltr" of type "style"

avatar Fedik
Fedik - comment - 29 Jun 2021

looks good on quick look,
I try to test

avatar Fedik
Fedik - comment - 29 Jun 2021

you probably forgot to run "npm install" ,
or just copy build/media_source/system/joomla.asset.json => media/system/joomla.asset.json
then it will work

9a9d8c7 29 Jun 2021 avatar laoneo Copy
avatar laoneo
laoneo - comment - 29 Jun 2021

Thanks @Fedik!!
So what is missing here is that beside the variables from the file https://github.com/joomla/joomla-cms/pull/34621/files#diff-5261142b249534c0e5d720df7c6ba474b9d920a0329745d37fd0dfb0f1bd8405 the core needs also the bootstrap color variables from here https://github.com/joomla/joomla-cms/blob/4.0-dev/administrator/templates/atum/scss/_variables.scss#L60. This needs to either properly be documented or we have to make the file build/media_source/system/css/global-variables.css a scss and include the bS variables for a proper default. Suggestions?

avatar wilsonge
wilsonge - comment - 29 Jun 2021

OK I'll ask the dumb question :) Is there a reason these need to be in a dedicated file in the system directory and not just overridden on a template basis in user.css. If this was all our CSS vars I'd kind of get it. But we're still going to have all the boostrap vars on a template basis.

avatar ciar4n
ciar4n - comment - 29 Jun 2021

A different approach would be to give each component its own CSS variables that the template can dial into.

Example...

// build/media_source/com_media/scss/components/_media-toolbar.scss

.media-toolbar {
  color: var(--com-media-toolbar-color, #333);
}

The template can then redefine the above variable, if they wish to do so, to their own template variable.

//  administrator/templates/atum/scss/style.scss

:root {
  --com-media-toolbar-color: var(--atum-bg-dark-60);
}
avatar laoneo
laoneo - comment - 30 Jun 2021

@ciar4n from separation of concern point of view this would be the right approach. But what we define here are some common variables which are used all over core to have a consistent look.
So I would rather do:

// build/media_source/com_media/scss/components/_media-toolbar.scss

.com-media {
  --com-media-toolbar-color: var(--template-bg-dark-60);
}

.media-toolbar {
  color: var(--com-media-toolbar-color);
}

In template:

//  administrator/templates/atum/scss/style.scss

.com-media {
  --com-media-toolbar-color: var(--template-bg-blue);
}
avatar laoneo
laoneo - comment - 30 Jun 2021

OK I'll ask the dumb question :) Is there a reason these need to be in a dedicated file in the system directory and not just overridden on a template basis in user.css. If this was all our CSS vars I'd kind of get it. But we're still going to have all the boostrap vars on a template basis.

That's my question too. How do we define all vars? What we can do is somehow include the BS vars in this file or mention in the file build/media_source/system/css/global-variables.css that the BS colors need to be included as well. I guess this is personal preference and everybody has it's own opinion. I would let the release leader decide how we are going to mention/reference the BS vars and then we can move forward as there are pros/cons for every approach.

avatar wilsonge
wilsonge - comment - 4 Jul 2021

@laoneo #34621 (comment) i like this - it seems fairly logical. although all extensions are going to be hardcoded into bootstrap anyhow for some of the sass functions.

avatar laoneo
laoneo - comment - 4 Jul 2021

And this is not the scope of this pr anyway. But definitely something we might consider for the future.

avatar wilsonge
wilsonge - comment - 4 Jul 2021

I don't think this dedicated SCSS file is the way to go anyhow. If we're going to allow people to override it should be in user.css i think. Can we revert that out and just rename the existing vars for now please?

avatar laoneo
laoneo - comment - 4 Jul 2021

So you want to have the variables inline on cassiopeia? How should the variables be renamed?

avatar brianteeman
brianteeman - comment - 7 Jul 2021

There are two different parts to this PR. The first is simply renaming variables from atum to template and I really don't have an opinion about that. The second is the creation of the build/media_source/system/css/global-variables.css and I think its purpose is not fully understood.

There are various times that backend code is used in the frontend, such as media manager and editing an article. The problem currently is that in these cases the backend code is not styled correctly as it is expecting the value of various css variables that are not present in the frontend template. (there are numerous issues reported)

One solution (and I think this was @laoneo first proposal) would be to add each of these variables to the frontend template inline in the index.php of the template. This works but I believe it is short sighted and is where I see a problem that this PR addresses - not many people will use cassiopeia.

With the inline solution it requires every template to add these variables in the template which might not seem for an obvious reason to many people. Even if we can resolve the education issue we come to the main problem. Updating the variables or even adding new ones in future joomla releases. If we ever have to do that then every template will need to be manually updated with the new code. It cannot be automated in any way.

With the solution proposed here it only requires the template to include the global-variables. If we ever have to update, modify, add new variables etc then the site owner will gain the benefit of those changes automatically on update.

We have a similar scenario in Joomla 3 which can be seen in the core beez3 template where we load templates/system/css/system.css

I hope that helps to explain.

avatar laoneo
laoneo - comment - 8 Jul 2021

@wilsonge and @brianteeman I let you guys find an agreement as I don't care if they are inline or in a file. For me is important to get this pr merged as base to fix the media manager on the front end.

avatar brianteeman
brianteeman - comment - 13 Jul 2021

@wilsonge thoughts? This really needs resolving as it solves numerous issues. Would be shame to waste another RC

avatar laoneo
laoneo - comment - 14 Jul 2021

So I reverted as I'm tired of waiting. Lets get this one merged and then we can fix the Media manager in #34624.
@brianteeman you can do then a followup pr with the var file, which is imo still a good option.

avatar brianteeman
brianteeman - comment - 14 Jul 2021

disappointing but obviously I feel your frustration at the lack of response

avatar wilsonge wilsonge - change - 15 Jul 2021
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2021-07-15 23:12:56
Closed_By wilsonge
avatar wilsonge wilsonge - close - 15 Jul 2021
avatar wilsonge wilsonge - merge - 15 Jul 2021
avatar wilsonge
wilsonge - comment - 15 Jul 2021

I definitely prefer @ciar4n 's proposed solution ( #34621 (comment) ) to the global variables file.

Add a Comment

Login with GitHub to post a comment