Skip to content

[4.0] favicon changes to support child templates#30388

Merged
wilsonge merged 5 commits intojoomla:4.0-devfrom
dgrammatiko:4.0-dev-child-favIcon
Sep 4, 2020
Merged

[4.0] favicon changes to support child templates#30388
wilsonge merged 5 commits intojoomla:4.0-devfrom
dgrammatiko:4.0-dev-child-favIcon

Conversation

@dgrammatiko
Copy link
Contributor

Pull Request for Issue # .

Summary of Changes

Cascade correctly for any parent/children template

Testing Instructions

Checkout this PR or download the installable at the end of the GitHub page

Check that both Cassiopeia and Atum have correct favicons
Install the templates from the PR #30192 (links in the description) and check that both parent and child templates have correct favicon (check the url)

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Documentation Changes Required

@Quy
Copy link
Contributor

Quy commented Aug 17, 2020

Object not found. Have not installed templates from the other PR yet.

Before PR:
<link href="/administrator/templates/atum/favicon.ico" rel="shortcut icon" type="image/vnd.microsoft.icon">

After PR:
<link href="/templates/atum/favicon.ico" rel="shortcut icon" type="image/vnd.microsoft.icon">

@dgrammatiko
Copy link
Contributor Author

@Quy you're right the logic was wrong, fixed with 93d707d

Co-authored-by: Quy <quy@fluxbb.org>
@Quy
Copy link
Contributor

Quy commented Aug 20, 2020

I have tested this item ✅ successfully on a15b321


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

1 similar comment
@laoneo
Copy link
Member

laoneo commented Aug 21, 2020

I have tested this item ✅ successfully on a15b321


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

@Quy
Copy link
Contributor

Quy commented Aug 21, 2020

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 21, 2020
@SharkyKZ
Copy link
Contributor

FWIW, using media folder for template assets is a mistake.

@dgrammatiko
Copy link
Contributor Author

FWIW, using media folder for template assets is a mistake.

Can you elaborate why?

@laoneo
Copy link
Member

laoneo commented Aug 23, 2020

Because CSS and JS files are not media.

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented Aug 23, 2020

Because CSS and JS files are not media.

AFAIK the majority (if not all) of the contents of the media folder is either css or js or images therefore they shouldn't be stored on that folder? Sorry I'm not getting it, for Joomla css, js, images, fonts, et al are static files. To my understanding media is what in other platforms is called assets, eg stored files needed for the rendering of the page

@SharkyKZ
Copy link
Contributor

It's not about the folder name. There are reasons why template assets are treated differently than assets of other extensions. For instance, they should be final and non-overridable. If they're placed in media folder, they become overridable. The magic code in Joomla\CMS\HTML\HTMLHelper looks for overrides in template directory which adds some unneeded file lookups. Before #30192 something like this could have made sense. I.e. having all template assets (including overrides) in media folder and user-defined overrides in template folder. But now there are also child template folders - both in media and template directory which just makes things slow and confusing.

@dgrammatiko
Copy link
Contributor Author

There are few misconceptions here:

For instance, they should be final and non-overridable. If they're placed in media folder, they become overridable.

That's wrong. The new mode templates (eg the ones that support Childs) have both their assets and the overriding folders on the media template therefore folders line css, js and images are indeed final and non-overridable (src === override)

looks for overrides in template directory which adds some unneeded file lookups.

The only extra lookups are for the child templates. There won't be a lookup eg for a current template in any of the media/templates/... Actually in order any of this to work the template needs to opt in with an <inheritable>1</inheritable>

But now there are also child template folders - both in media and template directory which just makes things slow and confusing.

Let me write this one more time: Templates that support child templates use ONLY the media folder for their OWN assets but ALSO for the OVERRIDES. Static assets for those templates (and their Childs) EXISTS ONLY IN THE MEDIA FOLDER

@SharkyKZ
Copy link
Contributor

SharkyKZ commented Aug 24, 2020

OK, you're right about performance. Not a fan of these differences between normal and inheritable templates though.

Are you going to expose media folder in com_templates? Template assets should be editable through Template Manager. I didn't see anything related to this in #30359.

@dgrammatiko
Copy link
Contributor Author

Template assets should be editable through Template Manager.

Yes that's the plan. Everything will be as before with the slight difference that ALL static assets will live in the media folder. UX should be the same from a user's GUI perspective. For devs the only hurdle is that they have to slightly adjust their packaging so particular files/folders end up in the media folder. I know that people hate changes so you have every right to hate/blame me for this

@wilsonge wilsonge merged commit 4b9c240 into joomla:4.0-dev Sep 4, 2020
@wilsonge
Copy link
Contributor

wilsonge commented Sep 4, 2020

Thanks!

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Sep 4, 2020
@wilsonge wilsonge added this to the Joomla 4.0 milestone Sep 4, 2020
dgrammatiko added a commit to dgrammatiko/joomla-cms that referenced this pull request Sep 29, 2020
…om_templates

* '4.0-dev' of github.com:joomla/joomla-cms: (70 commits)
  [4.0] Child templates consistency (joomla#30387)
  [4.0] favicon changes to support child templates (joomla#30388)
  [4.0] Update Readme for Api tests (joomla#30539)
  [4.0] [Multilingual Status module] Adding displaying a possible error if URL Language Code is empty (joomla#30537)
  [4.0] Display of horizontal mod_articles_news module (joomla#30527)
  [4.0] Useless installation lang strings (joomla#30568)
  [4.0] Numbers not digits (joomla#30559)
  [4.0] Accessibility plugin position (joomla#30552)
  [4.0] fix for inherit fields (joomla#30557)
  [4.0] Redundant words (joomla#30555)
  add missing legend to fieldset (joomla#30528)
  [4.0] [a11y] add statement on found results (joomla#30535)
  [4.0] com_finder ul instead of dl for easier styling (joomla#30534)
  [4.0] Messages/Alerts: using icons instead of text as heading (joomla#30516)
  [4.0] Increase API Test Coverage (joomla#26722)
  [4.0] Implementing display of password requirements for frontend (joomla#30473)
  [4.0] FieldsHelper: Choose a first available category  correctly (joomla#30268)
  Sort options (joomla#30531)
  Clear checkboxes on back button (joomla#30498)
  Update _icomoon.scss (joomla#30436)
  ...
sakiss pushed a commit to sakiss/joomla-cms that referenced this pull request Oct 16, 2020
@dgrammatiko dgrammatiko deleted the 4.0-dev-child-favIcon branch April 18, 2021 10:29
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.

6 participants

Comments