Skip to content

Latest commit

 

History

History
123 lines (82 loc) · 3.34 KB

plip-1653-review-ale-rt.rst

File metadata and controls

123 lines (82 loc) · 3.34 KB
Raw

PLIP 1653: Restructure CMFPlone static resources

PLIP ticket
plone/Products.CMFPlone#1653
Review by
Alessandro Pisa (alessandro.pisa@gmail.com)
Environment

The PLIP was reviewed using:

  • Kubuntu 18.10 64-bit
  • Python 2.7.15
  • Chrome 70.0.3538.110 (64-bit)
  • yarn 1.12.3
Date
November 26, 2018

Jenkins jobs

The plip buildout jobs work good:

PLIP buildout

  • Set up the buildout using the PLIP's config:

    $ cd plips && ../bin/buildout -c plip-1653-staticresources.cfg

Works good but we have the missing pin: profilehooks = 1.8.1

I had to manually edit:

  • parts/instance/etc/package-includes/998-resources-configure.zcml

to transform the relative path that points to the resource folder in to an absolute one.

Involved Packages and its pull requests

  • jquery.recurrenceinput.js: I created a simple PR (collective/jquery.recurrenceinput.js#32) that looks like can be merged pretty soon without waiting for the other plip packages. This is checked out in the src directory even if it is not an egg. Unsure how it is pulled in.
  • mockup: PR plone/mockup#874 had to be updated because of conflicting changes. Running make works until the very end but then rasises this error: plone/mockup#874 (comment) Browsing the docs complains about missing react.
  • plone.app.theming: plone/plone.app.theming#149 Mostly removal of old resources. Looks good.
  • plone.resourceeditor: plone/plone.resourceeditor#22 Mostly removal of old resources. Looks good.
  • plone.staticresources: Found some issues, most of them are already resolved. Code looks good. There is still an issue compile the bundle with Python3 (plone/plone.staticresources#4)
  • Products.CMFPlone: Mostly removal of old resources. Looks good.

Manual testing

I created a fresh Plone instance and browsed it checking that the styles were properly applied and that no JavaScript console was present.

I notice that I am getting 404 when getting: http://0.0.0.0:8080/plonejsi18n?domain=widgets&language=en_US

Still unsure if this is also a problem on current master.

Code review

Code looks nice and basically it is code moved from one package to another.

Test coverage is given.

Versions

All the packages involved have breaking changes and will be compatible only be compatible with Plone >= 5.2. Only jquery.recurrenceinput.js can have a feature release.

Documentation

It seems to me that the documentation has to be rechecked.

Conclusion

The framework team should discuss if, given the alpha state of Plone 5.2, this PLIP can be merged right now and apply fixes incrementally. My wish is that at least the Makefile for mockup will work as expected. It might be a good idea to take some time to do this in some of the sprints. A good candidate can be the Alpine city sprint.