? Pending

User tests: Successful: Unsuccessful:

avatar brianteeman
brianteeman
30 Dec 2019

see screenshots for changes

If we wait for an expert to fix it then it will never happen

Before

image

After

image

avatar brianteeman brianteeman - open - 30 Dec 2019
avatar brianteeman brianteeman - change - 30 Dec 2019
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 30 Dec 2019
Category Administration Templates (admin) Repository NPM Change
avatar infograf768
infograf768 - comment - 30 Dec 2019

Will test tomorrow. For the moment just a general remark.
It looks that the system searchtools.css is loaded differently in ltr and rtl vs the specific atum searchtools.scss which is included into atum template.css.
I discovered that when trying to solve the rtl float depending on browser window size for the client field In the installed language manager. It is extremely confusing and it may be that we have to fully understand what is going on here before touching again searchtools.

avatar brianteeman brianteeman - change - 30 Dec 2019
Labels Added: NPM Resource Changed ?
avatar infograf768
infograf768 - comment - 31 Dec 2019

@dgrammatiko
Can you help solving the loading order of rtl template css and system searchtools.css?
It also looks that it would be much more simple to do all the searchtools css only in the template block scss and get rid of the system one as it is extremely confusing to deal with both... We never know which one takes precedence over the other, etc.

avatar dgrammatiko
dgrammatiko - comment - 31 Dec 2019

@infograf768 what exactly is broken?
About returning back to monolithic css: you can revert my pr...

avatar infograf768
infograf768 - comment - 31 Dec 2019

@dgrammatiko
I did not say return to monolithic css. Using blocks _searchtools.scss is fine
The problem here is that we have 2 places where the searchtools css are fetched from

  1. _searchtools.scss from Atum blocks creates css in the template.
  2. A general searchtools.css in media/system/css/

It happens, as stated above, that in rtl, parts of the media/system/css/ searchtools.css is loaded after the template searchtools classes for some cases, creating the issue
Do we really need that file is the question? Or at least can you solve the issue?

LTR
Screen Shot 2019-12-31 at 12 19 21

RTL

Screen Shot 2019-12-31 at 12 20 29

The css in the template is created by the scss

  .js-stools-container-selector,
  .js-stools-container-selector-first {
     @include media-breakpoint-down(md) {
        float:none;
        width:100%;
        margin-right:0.5rem;
    }

Which should override the searchtools.css as it has not specific direction
It does for ltr, not for rtl.

In the searchtools.css in system we have

.js-stools .js-stools-container-selector {
	float: left;
	margin-right: auto;
}

and further

html[dir=rtl] .js-stools .js-stools-container-selector {
	float: right;
	margin-left: auto;
	margin-right: 0;
}

The only solution I found was again to add a css in the system one using html[dir=rtl] and @media etc.
That is really bothering as it really look as a hack.

avatar dgrammatiko
dgrammatiko - comment - 31 Dec 2019

@infograf768 on my mobile, so not much I can do right now but I just saw that there is a _searchtools.scss in the sass/blocks. This is utterly wrong! Rename it to searchtools.scss and move it in the parent dir next to template.scss. Also add an @import path/to/system/searchtools.css at the top. That should fix it

avatar infograf768
infograf768 - comment - 1 Jan 2020

@dgrammatiko
Sorry to say, but I tried as you proposed and it does not work.
First I had to also modify template.scss to use
@import "searchtools"; insteaf of @import "blocks/searchtools"; as the file is moved.

The path I added is @import "../../../../media/system/css/searchtools.css";
It does not look like working. Can one really import a .css file into a .scss file?
I tried it in template.scss as well as in the searchtools.scss without luck.
only result is that searchtools.css is loaded twice.

Also remains in joomla.asset.json this weird line

      "css": [
        "system/searchtools.css"
      ]

The whole thing is too confusing for me.

I also still don't understand the presence of 2 different css files for searchtools, i.e. as I guess media/system/css/searchtools.css has to be loaded whatever the admin template, what should it exactly contain that can be customised/overriden in the atum searchtools.scss ?

avatar infograf768
infograf768 - comment - 1 Jan 2020

hmm
We should not use the .css suffix when importing
Should be
@import "../../../../media/system/css/searchtools";

Also remains to solve the issue in rtl, i.e. decide what should exactly contain media/system/css/searchtools.css vs the template-rtl.css

avatar dgrammatiko
dgrammatiko - comment - 1 Jan 2020

First I had to also modify template.scss to use @import "searchtools"; insteaf of @import "blocks/searchtools"; as the file is moved.

You don't have to do that! What I proposed should just override the default loaded css, which is the correct way. What you are doing right now is loading the module css and then overriding it in the monolithic template.css. That's not how you suppose to write css in 2020. Happy new year...

Can one really import a .css file into a .scss file?
Yes, just remove the extension, IIRC, but you solved that already.

Just leave it as a separate file in the scss folder, remove the import from the template, and maybe add the override in the assets.JSON, you'll have to check how this new system works

avatar infograf768
infograf768 - comment - 1 Jan 2020

I am lost...

avatar infograf768
infograf768 - comment - 2 Jan 2020

@dgrammatiko

That's not how you suppose to write css in 2020. Happy new year...

Please do the PR. I am failing miserably, and it is not because I try to code in a pre-2020 way. It is because I mistake following your suggestions, or that I don't understand the details of them, or because something is wrong in your suggestions.

avatar dgrammatiko
dgrammatiko - comment - 2 Jan 2020

@infograf768 well, it's a bit more complicated and I should have mentioned that #25775 and #27268 are prerequisites here as the asset manager in its current form is a bit broken for these kind of operations. Ask someone with merging rights to push those 2. Then my comments will suddenly make much more sense

avatar infograf768
infograf768 - comment - 2 Jan 2020

I do have merging rights but I don’t usually merge something I don’t understand. It is the case for these.

avatar Fedik
Fedik - comment - 3 Jan 2020

if you need to drop/overide of use system/css/searchtools, you can override the asset (searchtools asset) definition for active template,
add to administrator/templates/atum/joomla.asset.json

{
    "name": "searchtools",
    "dependencies": [
      "core"
    ],
    "js": [
      "media/system/js/searchtools.min.js"
    ],
    "css": []
},

note "css": [] is empty, this will remove default searchtools.css from being loaded

avatar dgrammatiko
dgrammatiko - comment - 3 Jan 2020

note "css": [] is empty, this will remove default searchtools.css from being loaded

I think this needs to point to the templates/atum/css/searchtools.min.css, we are not supposed to remove the css but rather use the css that is tightly coupled to this particular template. The point is that this css file should only be present on the pages that actually have searchtools, I don't know if asset manager will take care of that, will it?

avatar Fedik
Fedik - comment - 3 Jan 2020

for path templates/atum/css/searchtools.min.css it then:

{
    "name": "searchtools",
    "dependencies": [
      "core"
    ],
    "js": [
      "media/system/js/searchtools.min.js"
    ],
    "css": [
      "searchtools.min.css"
    ]
},

it equal for JHtml::_("stylesheet", "searchtools.min.css" .... );

avatar Fedik
Fedik - comment - 3 Jan 2020

The point is that this css file should only be present on the pages that actually have searchtools

All in joomla.asset.json it just a definitions, it not loaded if you not call it to load somewhere in the code with $assetManager->useAsset('blabla').
So it will be used only when a searchtool layout call to $assetManager->useAsset('searchtools').

avatar infograf768
infograf768 - comment - 3 Jan 2020

At this time, we load for Atum both the blocks/searchtools.scss which goes into the template.css and therefore into template-rtl.css, AND media/system/css/searchtools.css

I still have no idea what should be loaded and what should be or not overriden.
Should the system one be overriden and in that case by what, AND if it should, what shall it contain?

We obviously can’t go on trying solving various display issues in rtl as well as ltr in the present situation.

Please make a PR and tell us where we should add/delete/modify.

avatar Fedik
Fedik - comment - 3 Jan 2020

we load for Atum both the blocks/searchtools.scss which goes into the template.css and therefore into template-rtl.css, AND media/system/css/searchtools.css

so all styling for searchtools already done in the Atum template(-rtl).css ?
then we do not need to load core searchtools.css at all, then use my first suggestion, which disable core searchtools.css, with "css": []

upd:

Please make a PR

I will try, a bit later

avatar dgrammatiko
dgrammatiko - comment - 3 Jan 2020

so all styling for searchtools already done in the Atum template(-rtl).css ?
then we do not need to load core searchtools.css at all, then use my first suggestion, which disable core searchtools.css, with "css": []

Sorry, but this is WRONG! The template SHOULDN'T be bloated. Each part of the Joomla (form element, search tools, etc) has it's own css. We have to keep this modularity, packing a 1/2 MB css file is not good, but the worst part is that people copy what the core is doing and they blindly reproduce it in their sites. So, with that in mind, please keep things modular...

avatar Fedik
Fedik - comment - 3 Jan 2020

yeah I know,
I have answered for current state

avatar infograf768
infograf768 - comment - 4 Jan 2020

so all styling for searchtools already done in the Atum template(-rtl).css ?
then we do not need to load core searchtools.css at all, then use my first suggestion, which disable core searchtools.css, with "css": []

In fact not. Not all styling is done in the template. It is done in an impredictable way both in the template.css (or RTL) AND the system searchtools.css.

For example we have for the LTR

/* Fixed filter fields (selector) */
.js-stools .js-stools-container-selector {
	float: left;
	margin-right: auto;
}

from the system searchtools.css
and

  .js-stools-container-selector,
  .js-stools-container-selector-first  {
     @include media-breakpoint-down(sm){
        float: none !important;
        width: 100%;
        margin-right: 0;
    }

from the template through the blocks searchtools.scss (therefore in the template.css for now)

This is why I am insisting, in case it is decided to load only the specific css for ATUM (separated or not), to know what should exactly contain the system one...

avatar dgrammatiko
dgrammatiko - comment - 4 Jan 2020

In fact not. Not all styling is done in the template. It is done in an impredictable way both in the template.css (or RTL) AND the system searchtools.css.

The root of the problem lies in:

  • not overriding properly the original css, templates should do exactly that not patching over other css
  • the source of the searchtools doesn't exist in scss, meaning you could not do all the fancy stuff that the preprocessor offer (eg extend). For every .css file in the build/media_source someone needs to do the conversion to scss files...

Final thought, do you really need an extra .rtl.css ? I mean you could do

.class {
/* Normal LTR code */
}

html[dir='rtl'] .class {
/* Only RTL */
}
avatar infograf768
infograf768 - comment - 4 Jan 2020

Final thought, do you really need an extra .rtl.css ? I mean you could do etc.

It is easy to add rtl the way below in the blocks/scss/ files and not creating fully new cascading series which would make the template.css enormous.
Separating all blocks scss into their smaller .css and not grouping the classes in a unique file would not solve the issue at stake anyway.

#content {
  .menu-quicktask {
    position: absolute;

    [dir=ltr] & {
      right: 1.25rem;
    }

    [dir=rtl] & {
      left: 1.25rem;
    }
  }
}

or

.dropdown-header {
      color: $white;
      background-color: var(--atum-bg-dark);
      box-shadow: $atum-box-shadow;
      font-size: inherit;
      padding: 1rem 0.75rem;

      [dir=rtl] & {
        text-align: right;
      }
    }

template-rtl.css contains 257 lines of classes that would have to be overriden or just created in the various scss, mostly in _global.scss
Quite a refactoring job.

But this is not the main point here... and my question receives no answer.
I therefore repeat it:

This is why I am insisting, in case it is decided to load only the specific css for ATUM (separated or not), to know what should exactly contain the system one...

avatar dgrammatiko
dgrammatiko - comment - 4 Jan 2020

to know what should exactly contain the system one

I'm not the one designed or coded that thing so I cannot answer that. I think you should ping other people for that answer...

avatar Fedik
Fedik - comment - 4 Jan 2020

yeah, the solution: or move all from searchtools.css to atum/css/searchtools.css or vice versa

I tried here 4.0-dev...Fedik:searchtools-move-to-tmpl
It just a "blind copy" of searchtools.css and blocs/_searchtools.css to atum/css/searchtools.css

But it really better to do by someone who know the template.

avatar dgrammatiko
dgrammatiko - comment - 4 Jan 2020

@Fedik that's a good starting point

avatar infograf768
infograf768 - comment - 4 Jan 2020

Will test tomorrow and try to modify the blind copy to something that makes sense by combining classes for ltr and rtl.
(If I understand well, we would definitely get rid of media/system/css/searchtools.css)

avatar infograf768
infograf768 - comment - 6 Jan 2020

@Fedik
here is a consolidation of the new searchtools.scss to replace the one you have in your branch.
Please make PR.
searchtools.scss.zip

Note1: @brianteeman
It includes what concerns searchtools (not toolbar) in this PR, as well as the modifications in #27388

Note2: There will still be changes to do in toolbar, both for ltr and ltr, specially for mobile.
Once @Fedik future PR is merged it will be easier to do the modifications

Note3: It looks that some classes may not be of use anymore in the new scss, but hard to find out as they are also defined in the js.

avatar Fedik
Fedik - comment - 6 Jan 2020

@infograf768 there it is #27416
I have copied your searchtools.scss.zip file to it

It looks that some classes may not be of use anymore in the new scss, but hard to find out as they are also defined in the js.

they may be from j3, that almost not possible to find,
in theory can just remove them and see where it will explode ?

avatar infograf768
infograf768 - comment - 6 Jan 2020

See #27416

@brianteeman
Please take off the searchtools part from this PR as it is included in #27416

avatar brianteeman brianteeman - change - 6 Jan 2020
Title
[4.0] Fix RTL toolbar and searchtools css
[4.0] Fix RTL toolbar
avatar brianteeman brianteeman - edited - 6 Jan 2020
avatar joomla-cms-bot joomla-cms-bot - change - 6 Jan 2020
Category Administration Templates (admin) Repository NPM Change Administration Templates (admin)
avatar brianteeman
brianteeman - comment - 6 Jan 2020

done as requested

avatar brianteeman brianteeman - change - 7 Jan 2020
Labels Removed: NPM Resource Changed
c2d612b 7 Jan 2020 avatar brianteeman cs
avatar dgrammatiko
dgrammatiko - comment - 7 Jan 2020

Since #27416 was merged I guess also this one should follow the same path, eg place the scss in the right folder, remove the _ and do the needed changes in the template.asset.json. Let's modularise instead of throwing everything in one file

PS the file that this code is overriding is:

joomla-toolbar-button .dropdown-item:focus,

avatar infograf768
infograf768 - comment - 7 Jan 2020

@dgrammatiko
We should do that in a specific pr and not only for that blocks/_toolbar.scss
For the moment I am merging this as we need it and, contrary to searchtools we have no confusion with another toolbar.css in media

avatar infograf768 infograf768 - change - 7 Jan 2020
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2020-01-07 11:13:36
Closed_By infograf768
avatar infograf768 infograf768 - close - 7 Jan 2020
avatar infograf768 infograf768 - merge - 7 Jan 2020
avatar infograf768
infograf768 - comment - 7 Jan 2020

Tks

avatar brianteeman
brianteeman - comment - 7 Jan 2020

thanks

Add a Comment

Login with GitHub to post a comment