Allow bundles to be rendered after "theme" and "custom" bundles. #4054

thet · 2024-11-08T18:49:53Z

JS and CSS resources can now be rendered after all other resources in their
resource group including the theme (e.g. the Barceloneta theme CSS).

There is a exception for custom CSS which can be defined in the theming
controlpanel. This one is always rendered as last style resource.

To render a resource after all other give it the "depends" value of "all". This
indicates that the resource depends on all other being rendered before. The
resource is then rendered as last resource of it's resource group.

This allows to override a theme with custom CSS from a bundle instead of having
to add the CSS customizations to the registry via the "custom_css" settings.
As a consequence, theme customization can now be done in the filesystem in
ordinary CSS files instead of being bound to a time consuming workflow which
involces upgrading the custom_css registry after every change.

Docs

plone/documentation#1802
plone/documentation#1803

mister-roboto · 2024-11-08T18:50:03Z

@thet thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

thet · 2024-11-08T18:52:12Z

Products/CMFPlone/resources/browser/resource.py

+        # Allow bundles to have "theme" resp. "theme" and "custom" as dependency
+        # so that bundles can be rendered after theme and custom resources.
+        js_names += ["theme"]
+        css_names += ["theme", "custom"]


See below in https://github.com/plone/Products.CMFPlone/pull/4054/files#diff-9e095767a08db89a248c84c46d31c75898eed2d7dc40b0757a835bb2af0ed95bR143 and https://github.com/plone/Products.CMFPlone/pull/4054/files#diff-9e095767a08db89a248c84c46d31c75898eed2d7dc40b0757a835bb2af0ed95bR165

If "theme" or "custom" does not exist (both added in line 192+) the bundle dependency cannot be resolved and the bundle is not included in the rendered output.

thet · 2024-11-08T18:52:30Z

@jenkins-plone-org please run jobs

stevepiercy · 2024-11-08T20:47:18Z

This needs to be documented, but I'm not sure where. Perhaps in Theming of Classic UI? That section is so sad.

jensens

I think this is a good enhancement. Anyway, this needs some documentation, since it is not obvious.

jensens · 2024-11-09T11:53:52Z

This needs to be documented, but I'm not sure where. Perhaps in Theming of Classic UI? That section is so sad.

I think this should go into https://6.docs.plone.org/classic-ui/static-resources.html

petschki

Somehow I could not get this to work correctly. I checked out your branch and tested it with the plone-fullscreen bundle:

I added custom to depends, but the CSS was not added... also the .js is missing (because custom is only defined for CSS but the bundle has JS also)
I added theme to depends and get the following traceback:

Traceback (innermost last):
  Module ZPublisher.WSGIPublisher, line 181, in transaction_pubevents
  Module ZPublisher.WSGIPublisher, line 391, in publish_module
  Module ZPublisher.WSGIPublisher, line 285, in publish
  Module ZPublisher.mapply, line 98, in mapply
  Module ZPublisher.WSGIPublisher, line 68, in call_object
  Module zope.browserpage.simpleviewclass, line 44, in __call__
  Module Products.Five.browser.pagetemplatefile, line 127, in __call__
  Module Products.Five.browser.pagetemplatefile, line 59, in __call__
  Module zope.pagetemplate.pagetemplate, line 134, in pt_render
  Module Products.PageTemplates.engine, line 368, in __call__
  Module z3c.pt.pagetemplate, line 198, in render
  Module chameleon.zpt.template, line 328, in render
  Module chameleon.template, line 229, in render
  Module chameleon.utils, line 20, in raise_with_traceback
  Module chameleon.template, line 200, in render
  Module 1ac8f88237956af17a352f1fc406ccdd, line 1880, in render
  Module 985090d925cc459bf0e69fde89f3e030, line 903, in render_master
  Module 615bdf9739ecd0ffd4d4eee8b7a6ebd5, line 498, in render_master
  Module zope.contentprovider.tales, line 76, in __call__
  Module zope.viewlet.manager, line 157, in update
  Module zope.viewlet.manager, line 163, in _updateViewlets
  Module Products.CMFPlone.resources.browser.resource, line 260, in update
  Module webresource._api, line 719, in render
  Module webresource._api, line 678, in resolve
webresource._api.ResourceMissingDependencyError: webresource._api.ResourceMissingDependencyError: Resource defines missing dependency: <PloneScriptResource name="plone-fullscreen", depends="['theme']">

what am I missing here?

ale-rt · 2024-11-11T08:41:38Z

IMO custom should stay the last one.

petschki · 2024-11-11T08:44:05Z

IMO custom should stay the last one.

+1 I think that would be the expected behavior. If I change something TTW it should override everything else (without using !important all the time)

jensens · 2024-11-11T09:26:13Z

IMO custom should stay the last one.

+1 I think that would be the expected behavior. If I change something TTW it should override everything else (without using !important all the time)

I too agree

thet · 2024-12-03T16:30:38Z

@jenkins-plone-org please run jobs

…cluding default arguments.

thet · 2024-12-03T16:43:00Z

@jenkins-plone-org please run jobs

Bundles can now depend on the automatically added "theme" and "custom" bundles from plone.app.theming and be rendered after those (e.g. after the plonetheme.barceloneta CSS). This allows to override a theme with custom CSS from a bundle instead of having to add the CSS customizations to the registry via the "custom_css" settings. As a consequence, theme customization can now be done in the filesystem in ordinary CSS files instead of being bound to a time consuming workflow which involces upgrading the registry after every change.

thet · 2024-12-03T21:19:00Z

@jenkins-plone-org please run jobs

stevepiercy · 2024-12-04T00:02:25Z

Should I use the news item as the documentation for this new feature?

https://github.com/plone/Products.CMFPlone/pull/4054/files#diff-b79fc648ba65de1bd4da45d2bd3181b51ec6ff5b95fcb0cc91125609d52ad0fc

stevepiercy

While working on the documentation, I found a bug in the control panel that should be corrected either in this PR or a related one in the correct repository, if not this one.

Visit https://classic.demo.plone.org/@@theming-controlpanel#autotoc-item-autotoc-2 (Site Setup > Theming > Advanced settings > Custom Styles)
Click the link Theming documentation, which goes to a 404.

The correct link and label are Theming of Classic UI.

See plone/Products.CMFPlone#4054

stevepiercy · 2024-12-04T03:48:06Z

PR created to address the bug in the control panel UI at plone/plone.app.theming#248.

While I'm in there, should I update any other control panel message?

ale-rt · 2024-12-04T07:39:43Z

Products/CMFPlone/tests/testResourceRegistries.py

+    bundle.jscompilation = f"http://foo.bar/{name}.js"
+    bundle.csscompilation = f"http://foo.bar/{name}.css"
+    bundle.depends = depends
+    return bundle


What is the reason to move this code outside of the class?

because it's used in two different classes.
Keep it as it is or better move it back in?

But in the other class it is defined as a method... https://github.com/plone/Products.CMFPlone/pull/4054/files#diff-8ef40cff64bf27605c753443d198309a61a480a09ec62d66fed8152c607cb600R264

IMO leave it like it was or make the test classes inherit from a common class.

ale-rt · 2024-12-04T07:48:19Z

Products/CMFPlone/resources/browser/resource.py

+        # Fix "all" dependencies to be rendered after the last known non-all resource.
+        for resource in depends_all_js:
+            # Change all "all" to last known JS resource
+            dependencies = map(
+                lambda dep: dep if dep != "all" else last_js_bundle, resource.depends
+            )
+            # Remove None values, e.g. if no known JS resource was found
+            dependencies = filter(lambda dep: dep is not None, dependencies)
+            resource.depends = list(dependencies)


I think that use map and filter with two custom lambdas and then casting to a list can be replaced with a much more clear for cycle and probably performant for cycle.

Ref: plone/Products.CMFPlone#4054 Ref: plone/Products.CMFPlone#4074

Branch: refs/heads/master Date: 2024-12-03T19:42:58-08:00 Author: Steve Piercy (stevepiercy) <[email protected]> Commit: plone/plone.app.theming@1933dd8 Update link and label to Theming of Classic UI documentation See plone/Products.CMFPlone#4054 Files changed: M src/plone/app/theming/browser/controlpanel.pt Repository: plone.app.theming Branch: refs/heads/master Date: 2024-12-03T20:09:51-08:00 Author: Steve Piercy (stevepiercy) <[email protected]> Commit: plone/plone.app.theming@dc15462 Create 248.bugfix Files changed: A news/248.bugfix Repository: plone.app.theming Branch: refs/heads/master Date: 2024-12-04T06:32:57-08:00 Author: Steve Piercy (stevepiercy) <[email protected]> Commit: plone/plone.app.theming@907d61d Merge pull request #248 from plone/docs-classic-ui-theming Update link and label to Theming of Classic UI documentation Files changed: A news/248.bugfix M src/plone/app/theming/browser/controlpanel.pt

petschki

@thet I think we should rethink this and use the grouping api of webresource. If we move the resources with depends="all" to a new "deferred" ResourceGroup which gets resolved after the "theming" bundles then webresource does our work. See my experimental commit here: 92a82ec ...

ale-rt · 2024-12-05T08:01:03Z

Products/CMFPlone/resources/browser/resource.py

+            dependencies = map(
+                lambda dep: dep if dep != "all" else last_css_bundle, resource.depends
+            )
+            # Remove None values, e.g. if no known JS resource was found


Remove None values, e.g. if no known JS resource was found

This is for CSS right?
The code in this cycle and in the one above looks the same.
If the treatment for the resources is equal maybe it is better to move it to a method?

petschki · 2024-12-06T05:41:28Z

Closed this in favor of #4076

thet requested a review from petschki November 8, 2024 18:49

thet requested review from jensens and ale-rt November 8, 2024 18:50

thet commented Nov 8, 2024

View reviewed changes

jensens approved these changes Nov 9, 2024

View reviewed changes

petschki requested changes Nov 11, 2024

View reviewed changes

thet force-pushed the thet-bundle-after-theme branch from 0ffa287 to 758e21f Compare December 3, 2024 16:25

thet mentioned this pull request Dec 3, 2024

Allow bundles to be rendered after all other. #4074

Closed

Tests, resource registry: Factor out _make_bundle for better reuse in…

7043c53

…cluding default arguments.

thet force-pushed the thet-bundle-after-theme branch from 758e21f to d2c378f Compare December 3, 2024 16:36

thet force-pushed the thet-bundle-after-theme branch from d2c378f to ce0815c Compare December 3, 2024 21:17

stevepiercy requested changes Dec 4, 2024

View reviewed changes

stevepiercy added a commit to plone/plone.app.theming that referenced this pull request Dec 4, 2024

Update link and label to Theming of Classic UI documentation

1933dd8

See plone/Products.CMFPlone#4054

stevepiercy mentioned this pull request Dec 4, 2024

Update link and label to Theming of Classic UI documentation plone/plone.app.theming#248

Merged

stevepiercy mentioned this pull request Dec 4, 2024

Document how to specify a static resource bundle to load as the last … plone/documentation#1802

Merged

ale-rt reviewed Dec 4, 2024

View reviewed changes

thet added a commit to plone/documentation that referenced this pull request Dec 4, 2024

Document resource registry depends-all feature.

f1c76b6

Ref: plone/Products.CMFPlone#4054 Ref: plone/Products.CMFPlone#4074

thet mentioned this pull request Dec 4, 2024

Resource registry updates plone/documentation#1803

Closed

thet requested review from petschki, jensens, stevepiercy and ale-rt December 4, 2024 11:25

petschki requested changes Dec 4, 2024

View reviewed changes

ale-rt reviewed Dec 5, 2024

View reviewed changes

petschki mentioned this pull request Dec 5, 2024

Implement resource bundle depends on all to be rendered at last #4076

Merged

petschki closed this Dec 6, 2024

thet deleted the thet-bundle-after-theme branch December 6, 2024 09:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow bundles to be rendered after "theme" and "custom" bundles. #4054

Allow bundles to be rendered after "theme" and "custom" bundles. #4054

thet commented Nov 8, 2024 •

edited

Loading

mister-roboto commented Nov 8, 2024

thet Nov 8, 2024

thet commented Nov 8, 2024

stevepiercy commented Nov 8, 2024

jensens left a comment

jensens commented Nov 9, 2024

petschki left a comment •

edited

Loading

ale-rt commented Nov 11, 2024

petschki commented Nov 11, 2024

jensens commented Nov 11, 2024

thet commented Dec 3, 2024

thet commented Dec 3, 2024

thet commented Dec 3, 2024

stevepiercy commented Dec 4, 2024

stevepiercy left a comment

stevepiercy commented Dec 4, 2024

ale-rt Dec 4, 2024

thet Dec 4, 2024

ale-rt Dec 5, 2024

ale-rt Dec 4, 2024

petschki left a comment

ale-rt Dec 5, 2024

petschki commented Dec 6, 2024

Allow bundles to be rendered after "theme" and "custom" bundles. #4054

Allow bundles to be rendered after "theme" and "custom" bundles. #4054

Conversation

thet commented Nov 8, 2024 • edited Loading

Docs

mister-roboto commented Nov 8, 2024

thet Nov 8, 2024

Choose a reason for hiding this comment

thet commented Nov 8, 2024

stevepiercy commented Nov 8, 2024

jensens left a comment

Choose a reason for hiding this comment

jensens commented Nov 9, 2024

petschki left a comment • edited Loading

Choose a reason for hiding this comment

ale-rt commented Nov 11, 2024

petschki commented Nov 11, 2024

jensens commented Nov 11, 2024

thet commented Dec 3, 2024

thet commented Dec 3, 2024

thet commented Dec 3, 2024

stevepiercy commented Dec 4, 2024

stevepiercy left a comment

Choose a reason for hiding this comment

stevepiercy commented Dec 4, 2024

ale-rt Dec 4, 2024

Choose a reason for hiding this comment

thet Dec 4, 2024

Choose a reason for hiding this comment

ale-rt Dec 5, 2024

Choose a reason for hiding this comment

ale-rt Dec 4, 2024

Choose a reason for hiding this comment

petschki left a comment

Choose a reason for hiding this comment

ale-rt Dec 5, 2024

Choose a reason for hiding this comment

petschki commented Dec 6, 2024

thet commented Nov 8, 2024 •

edited

Loading

petschki left a comment •

edited

Loading