-
Notifications
You must be signed in to change notification settings - Fork 2
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
Integration of sc.recipe.staticresources for Plone 4 & 5 compatibility #45
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need an upgrade step to remove the CSS and JS registries entries; check collective.lazysizes code; we also need to process all CSS files and icons the same way.
@@ -0,0 +1,5 @@ | |||
<link rel="stylesheet" tal:attributes="href string:${view/site_url}/++resource++collective.glossary/main.css" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file can't be hardcoded, it needs to be generated by webpack also; check:
https://github.com/collective/collective.lazysizes/tree/4.1.4/webpack/app
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, this file should not be in the GIT repository, just ignore the whole static folder (except the usual README file that you should add in the repository before remove everything else).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and I don't get the point of main.css
file.. this should be incorporated into webpack generated file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is historical: the tooltip css code was only shown for "context/@@glossary_state".
I suppose that all the CSS can be merged into one file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merged all css in one file
@@ -0,0 +1,5 @@ | |||
<link rel="stylesheet" tal:attributes="href string:${view/site_url}/++resource++collective.glossary/main.css" /> | |||
<tal:if tal:condition="context/@@glossary_state"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of a tal
condition, we must handle this on the viewlet; it's easier and faster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry, I don't see how the JS could be conditionally served without using a tal
condition.
The tooltip must only be used in certain types as specified in control panel, so there has to be a condition for rendering the <script>
tag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added an ajax call in the javascript to deal with it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please look at my comments
@rodfersou can you please help @daggelpop fixing the viewlet when you have some spare time? I think that could be very important so more people understand this approach. best regards! |
Is Plone 4 compatibility still needed? At some point it should be dropped. |
Plone 4 compatibility is not the issue here, but the (sad) state of the resource registries in Plone 5. IMO, sc.recipe.staticresources is the best way to bring modern JS technologies into Plone. |
@hvelarde please take a look at my changes |
@@ -6,7 +6,7 @@ | |||
<!-- Basic metadata --> | |||
<property name="title" i18n:translate="">Term</property> | |||
<property name="description" i18n:translate="">A Term</property> | |||
<property name="content_icon">++resource++collective.glossary/term-icon.png</property> | |||
<property name="content_icon">++resource++collective.glossary/img/term-icon.png</property> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for some reason the portlet navigation don't find the icon in this new url (after run upgrade step).
need help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you sure icons are generated inside the img
directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rodfersou thank you for your help on this! I still have to take a look on it on my personal computer. for some unknown reason Travis was not working; we need to push some silly change to restart it. @daggelpop @mauritsvanrees if everything is fine we can have a release soon; we need to start checking compatibility with Python 3 and Plone 5.2. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for the delay on checking this; Iḿ not being able of running the upgrade step, this is what I idid:
- create a Plone 4.3 site from master branch and add a Glossary and a couple of Terms
- checkout this branch, run buildout and start Plone again
- an error is shown while trying to access the site:
SyntaxError: JSON.parse: unexpected character at line 2 column 1 of the JSON data
can you take a look on this, @rodfersou?
@daggelpop, @mauritsvanrees do you mind to make a similar test for Plone 5.1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested on brand new sites in both Plone 4.3 and Plone 5.1.
the refactor seems to be working, nevertheless I'm going to open a new issue that I consider a blocker for a new release as the upgrade step is not working for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rodfersou there's a test failing in Plone 4.3:
https://travis-ci.org/collective/collective.glossary/jobs/465459668#L4739
duplicate of #44 on a branch instead of a fork
Hi,
Following remarks on #43, I've started the Plone 5 adaptation from scratch.
I've used sc.recipe.staticresources, although I have to admit I'm not familiar with webpack and the current JS ecosystem.
So far, the addon works for me on Plone 4.3 & 5.1.
As this pull request clears, I'll make others for the features I presented in #43 (alternative rich text description, filter by letter bar, FR translations).