Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add custom.css resource after diazo bundle #3089

Merged
merged 3 commits into from
Apr 23, 2020
Merged

Add custom.css resource after diazo bundle #3089

merged 3 commits into from
Apr 23, 2020

Conversation

MrTango
Copy link
Contributor

@MrTango MrTango commented Apr 23, 2020

Insert virtual custom.css bundle after Diazo bundle.

@MrTango MrTango merged commit 4c77457 into master Apr 23, 2020
@MrTango MrTango deleted the custom_css_view branch April 23, 2020 15:39
@mauritsvanrees
Copy link
Member

For me it is fine to include this in 5.2. It makes small site-specific styling changes easier, which is good.
Does the @plone/framework-team want to weigh in on this?

The upgrade step would need to be backported to 5.2 then.

@ale-rt
Copy link
Member

ale-rt commented Apr 27, 2020

I added a note in the "FWT Meeting Protocol 2020-05-05" shared document!

@mauritsvanrees
Copy link
Member

I see the team meeting was supposed to be yesterday. Was this discussed?

(Needs an upgrade step in plone.app.upgrade for 5.2, like was added for 6.0 already.)

@ale-rt
Copy link
Member

ale-rt commented May 7, 2020

We rescheduled the meeting to next Tuesday (2020-05-12)!

),
'bundle': 'custom-css'
}
result.append(custom_css)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it is better to add some condition to not always append the custom css?
It makes no sense if it is empty, right?

@ale-rt
Copy link
Member

ale-rt commented May 26, 2020

The @plone/framework-team approved the backport of this PR to 5.2 https://community.plone.org/t/fwt-meeting-minutes-2020-05-26/12344

@mauritsvanrees
Copy link
Member

I am pretty sure this needs an upgrade step for <records interface="plone.app.theming.interfaces.IThemeSettings" /> to include the new field, like @MrTango did in https://github.com/plone/plone.app.upgrade/pull/230/files for Plone 6.0. But I don't see a problem on the theming control panel when switching from a version without this field to a version with this field. This surprises me.
Maybe I got confused when running into unrelated errors though.

@MrTango Could you check that? And could you backport this CMFPlone change from master to 5.2? (Otherwise I could do it.)

@mauritsvanrees
Copy link
Member

I checked again. Started with a fresh Plone 5.2.1 site, copied its database to coredev, ran the Plone upgrade. The theming control panel looked fine, but when I save the Advanced tab, with no changes, I get this error:

Traceback (innermost last):
  Module ZPublisher.WSGIPublisher, line 162, in transaction_pubevents
  Module ZPublisher.WSGIPublisher, line 359, in publish_module
  Module ZPublisher.WSGIPublisher, line 262, in publish
  Module ZPublisher.mapply, line 85, in mapply
  Module ZPublisher.WSGIPublisher, line 63, in call_object
  Module plone.app.theming.browser.controlpanel, line 65, in __call__
  Module plone.app.theming.browser.controlpanel, line 215, in update
  Module plone.registry.recordsproxy, line 59, in __setattr__
AttributeError: custom_css_timestamp

@mauritsvanrees
Copy link
Member

Ah, plone.app.theming has its own GS profile, and CMFPlone automatically applies its upgrade steps. So if we add an upgrade step to reload the settings, this fixes the problem. Done in plone/plone.app.theming#184 in this commit: plone/plone.app.theming@a360731.

Technically, the plone.app.upgrade changes for 6.0 could be reverted, but it's fine to leave them (and the part of those changes that creates a v60 structure is still needed anyway).

So that only leaves backporting the CMFPlone changes of the current PR to 5.2.

@mauritsvanrees
Copy link
Member

I made a PR: #3117

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.

3 participants