Skip to content

[4.0] Fix the vendor css for the template#26077

Merged
wilsonge merged 28 commits intojoomla:4.0-devfrom
dgrammatiko:scss_fix
Nov 30, 2019
Merged

[4.0] Fix the vendor css for the template#26077
wilsonge merged 28 commits intojoomla:4.0-devfrom
dgrammatiko:scss_fix

Conversation

@dgrammatiko
Copy link
Contributor

@dgrammatiko dgrammatiko commented Aug 29, 2019

Pull Request for Issue # .

Joomla has a very well established pattern for overridding CSS and JS assets. You place the override file in the template specific folder and magic happens.

For some reason, instead of promoting modularity and the core functionality we end up making the css of the template a huge garbage bin, where anyone can append some css.

This should stop as the produced css is not maintainable and is spaggetti code that nobody could figure out where could be the source of a bug (cascading is already hard let's not make it even harder). Not to mention that performance also has a negative impact but that's minor in this case...

Summary of Changes

All the files that are loaded through an HTMLHelper should NOT be overridden with code internally to the template. This PR reverts it for the following external resources (vendor folder):

  • awesomplete
  • choicesjs
  • font awesome
  • minicolors
  • joomla-alert
  • joomla-tab

Testing Instructions

Check if anything is broken in the backend
(it shouldn't be as I'm still importing the original css, I know but then this PR is becoming a major refactor...)

Expected result

Actual result

Documentation Changes Required

PS there should be some QA for the scss of the template, the code is nowhere near beta state, just saying

@ghost
Copy link

ghost commented Sep 2, 2019

Please resolve conflicting files so this PR can get tested at Worldwide Pizza, Bugs & Fun, October 19th

@ghost ghost added the Conflicting Files label Sep 2, 2019
@Quy
Copy link
Contributor

Quy commented Oct 8, 2019

FontAwesome icons don't appear.

26077

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented Oct 9, 2019

@Quy do you have the css file administrator/templates/atum/css/vendor/fontawesome-free/fontawesome.min.css ? If so do you also get the actual font?

@Quy
Copy link
Contributor

Quy commented Oct 9, 2019

Installed your branch. There is no vendor directory.

@C-Lodder
Copy link
Member

C-Lodder commented Oct 9, 2019

@Quy Did you run npm i?

@Quy
Copy link
Contributor

Quy commented Oct 9, 2019

Yes, both npm i and npm ci. Still no go.

@dgrammatiko
Copy link
Contributor Author

@Quy what's your OS?

@Quy
Copy link
Contributor

Quy commented Oct 9, 2019

Testing locally with XAMPP on Windows 10 Home.

@dgrammatiko
Copy link
Contributor Author

@Quy thanks, I'll check why the build tools are failing to generate the css for winDOS

@dgrammatiko
Copy link
Contributor Author

@Quy can you give it another try?

@Quy
Copy link
Contributor

Quy commented Oct 17, 2019

Almost there. Here are a few more that don't appear.

26077a
26077b

@dgrammatiko
Copy link
Contributor Author

@Quy thank you, should fine now

@Quy
Copy link
Contributor

Quy commented Oct 17, 2019

They show up but don't display properly.

26077c

@dgrammatiko
Copy link
Contributor Author

There are still two instances of choices.css loaded.

I know! The problem is that the original is loaded with webAssets, that for some reason doesn't pick up the override. If I try to override the css then the JS ends up not loading. I think @Fedik already has a PR that gives more granular access but till that PR is merged the original (from media) css file is also loaded here.

@Fedik
Copy link
Member

Fedik commented Oct 28, 2019

well, it sounds like it better to allow to override vendor files in "old way" also, for j4
I try to update that PR some time later, or I will do another one, will see

@dgrammatiko
Copy link
Contributor Author

@wilsonge is there any interest on this one or should I move on?

@SharkyKZ
Copy link
Contributor

SharkyKZ commented Nov 6, 2019

I have tested this item ✅ successfully on ab05717


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/26077.

1 similar comment
@Quy
Copy link
Contributor

Quy commented Nov 8, 2019

I have tested this item ✅ successfully on ab05717


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/26077.

@Quy
Copy link
Contributor

Quy commented Nov 8, 2019

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/26077.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Nov 8, 2019
@wilsonge wilsonge merged commit 7ad4f2a into joomla:4.0-dev Nov 30, 2019
@wilsonge
Copy link
Contributor

Thanks!

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Nov 30, 2019
@wilsonge wilsonge added this to the Joomla 4.0 milestone Nov 30, 2019
@infograf768
Copy link
Member

@dgrammatiko
Looks like atum specific choice.css is not loaded in com_associations.
Screen Shot 2019-12-01 at 17 02 26

@dgrammatiko
Copy link
Contributor Author

@infograf768 are the tags ok in that page?

@infograf768
Copy link
Member

Same.

Screen Shot 2019-12-01 at 17 44 54

@dgrammatiko
Copy link
Contributor Author

@infograf768 in the atum/component.php
add
after line 17

$wa    = $this->getWebAssetManager();

and after line 30

// Enable assets
$wa->enableAsset('template.atum.' . ($this->direction === 'rtl' ? 'rtl' : 'ltr'));
$wa->enableAsset('fontawesome-free');

// TODO: remove the following line whenever the assets are fixed to respect the ovverides
HTMLHelper::_('stylesheet', 'vendor/choicesjs/choices.css', array('version' => 'auto', 'relative' => true));

On a more serious note the assets for the iframes need some attention (eg the component.php needs to reflect the index.php)

@dgrammatiko
Copy link
Contributor Author

@infograf768 check #27187

@Fedik
Copy link
Member

Fedik commented Dec 1, 2019

$wa->enableAsset('fontawesome-free');

all you need:

$wa->enableAsset('fontawesome-free')->enableAsset('choices');

@dgrammatiko
Copy link
Contributor Author

@Fedik this didn't work as it expects me to override both the css and the javascript. I wanted only the css from the choices to be overriden

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

NPM Resource Changed This Pull Request can't be tested by Patchtester

Projects

None yet

Development

Successfully merging this pull request may close these issues.