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

Version 2.2.0 to 4.0.0 [braking change] #56

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

szakitibi
Copy link

Fixes #46

@@ -1,5 +0,0 @@
/*! jQuery UI - v1.10.4 - 2015-09-20
Copy link
Member

Choose a reason for hiding this comment

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

I think the code can be kept inside the js folder

Copy link
Author

Choose a reason for hiding this comment

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

I know it is a taste thing, but I do not like the mixed .js an .py files.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I'm sorry. I wasn't clear here. I'm talking precisely about the folder that contains .js files. Basically what I'm suggesting here is to rename the jquery-ui-1.13.2.custom folder to js.

@@ -0,0 +1,6 @@
/*! jQuery UI - v1.13.2 - 2023-02-09
* http://jqueryui.com
* Includes: widget.js, position.js, data.js, disable-selection.js, focusable.js, form-reset-mixin.js, jquery-patch.js, keycode.js, labels.js, scroll-parent.js, tabbable.js, unique-id.js, widgets/draggable.js, widgets/droppable.js, widgets/resizable.js, widgets/selectable.js, widgets/sortable.js, widgets/accordion.js, widgets/autocomplete.js, widgets/button.js, widgets/checkboxradio.js, widgets/controlgroup.js, widgets/datepicker.js, widgets/dialog.js, widgets/menu.js, widgets/mouse.js, widgets/progressbar.js, widgets/selectmenu.js, widgets/slider.js, widgets/spinner.js, widgets/tabs.js, widgets/tooltip.js, effect.js, effects/effect-blind.js, effects/effect-bounce.js, effects/effect-clip.js, effects/effect-drop.js, effects/effect-explode.js, effects/effect-fade.js, effects/effect-fold.js, effects/effect-highlight.js, effects/effect-puff.js, effects/effect-pulsate.js, effects/effect-scale.js, effects/effect-shake.js, effects/effect-size.js, effects/effect-slide.js, effects/effect-transfer.js
Copy link
Member

Choose a reason for hiding this comment

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

Did you add all the plugins? Wouldn't it be better to only add the plugins that defaulted to True in:

<records interface="collective.js.jqueryui.controlpanel.IJQueryUIPlugins">

Do you need all plugins?

Copy link
Author

Choose a reason for hiding this comment

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

Yes. All is there. How it is by default on https://jqueryui.com/download/
Since the registry based configuration is removed I went with the simplest solution.
Updated selection as suggested - see commit 1ff1cd8 and

* Includes: widget.js, position.js, data.js, disable-selection.js, focusable.js, form-reset-mixin.js, jquery-patch.js, keycode.js, labels.js, scroll-parent.js, tabbable.js, unique-id.js, widgets/draggable.js, widgets/droppable.js, widgets/resizable.js, widgets/selectable.js, widgets/sortable.js, widgets/accordion.js, widgets/button.js, widgets/checkboxradio.js, widgets/controlgroup.js, widgets/datepicker.js, widgets/dialog.js, widgets/mouse.js, widgets/progressbar.js, widgets/slider.js, effect.js, effects/effect-blind.js, effects/effect-bounce.js, effects/effect-clip.js, effects/effect-drop.js, effects/effect-explode.js, effects/effect-fade.js, effects/effect-fold.js, effects/effect-highlight.js, effects/effect-puff.js, effects/effect-pulsate.js, effects/effect-scale.js, effects/effect-shake.js, effects/effect-size.js, effects/effect-slide.js, effects/effect-transfer.js

Copy link
Member

Choose a reason for hiding this comment

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

I selected the plugins that were default in collective.js.jqueryui and the difference in size was small in relation to all plugins. So I changed my mind and I think it's not worth removing plugins. It would have to be documented which ones to select and would make things more difficult. Sorry for the inconvenience.


WARNINGS
========
Can not be upgraded from 2.2.0 directly! It is a major braking release!
Copy link
Member

Choose a reason for hiding this comment

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

I think it should be possible to do the update. An upgrade step should be created.


Plone < 4
Copy link
Member

Choose a reason for hiding this comment

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

Do not remove version information needed by older Plone.

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Even removing support for older versions of Plone, we should not remove the information of which version of collective.js.jqueryui is compatible with these versions. Note that today the product is no longer compatible with Plone < 4, but the information is still in the README. This makes it easy if some old Plone user arrives at the repository and wants to use the product on their site.

@@ -16,7 +16,7 @@

setup(
name="collective.js.jqueryui",
version="2.2.1.dev0",
version="4.0.0 (unreleased)",
Copy link
Member

Choose a reason for hiding this comment

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

Why not 3.0.0?

Copy link
Author

Choose a reason for hiding this comment

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

To point it out better, that it is a huge jump and a braking change. So much has been removed, that I think it is basically a new product, just to register the bundle.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that makes sense. Even if we were moving from 2.99 to 3.0.0, that would also indicate a breaking release. I think we can use 3.0.0.

"Framework :: Plone :: 5.0",
"Framework :: Plone :: 5.1",
"Framework :: Plone :: 5.2",
"Programming Language :: Python",
"Programming Language :: Python :: 2.7",
Copy link
Member

Choose a reason for hiding this comment

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

This product doesn't have much in python. I think compatibility with python 2 could be maintained, at least to have a compatible version that fixes the security problem.

@@ -1,19 +1,14 @@
# -*- coding: utf-8 -*-
Copy link
Member

Choose a reason for hiding this comment

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

I think it doesn't make sense to have a resource view anymore, since we can't select the plugins anymore.

@@ -1 +0,0 @@
# -*- coding: utf-8 -*-
Copy link
Member

Choose a reason for hiding this comment

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

I think the example could be maintained. It's a way to test the product.

@wesleybl
Copy link
Member

What exactly was done in this commit:

7c2cc3e

?

@szakitibi
Copy link
Author

What exactly was done in this commit:

7c2cc3e

?

See commit 98cd6f2, and the reasoning here

make it work
------------
The result of the unpacked jQuery UI zip with Plone 5.2.4 (5212) patterns require.js raises an error:
```
Uncaught Error: Mismatched anonymous define() module: function(V){"use strict";V.ui=V.ui||{};V.ui.version="1.13.2"; ... require.js:168
at makeError (require.js:168:17)
at intakeDefines (require.js:1254:36)
at Object.localRequire [as require] (require.js:1446:21)
at requirejs (require.js:1797:24)
at patterns-compiled.js:15444:1
```
Solution was to manually remove from `jquery-ui-1.13.2.custom/jquery-ui.min.js` the ADM part (`"function"==typeof define&&define.amd?define(["jquery"],t):`) right at the start of the javascript code:
`!function(t){"use strict";"function"==typeof define&&define.amd?define(["jquery"],t):t(jQuery)}(function(V){ ...`
so we end up with:
`!function(t){"use strict";t(jQuery)}(function(V){ ...`

@wesleybl
Copy link
Member

See commit 98cd6f2, and the reasoning here

hmm, I remember this problem. It would be good to change the UPGRADE.txt to .rst or .md to highlight the code that needs to be changed.

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.

Security: Upgrade to jQuery UI 1.13.0
2 participants