Solve issue 859 and 871 #872

Mubra · 2019-10-08T18:45:26Z

The solution was to change the selector "$ le" to "le" because it did not take the charged element, only those created.( closes #871 )
This indirectly closes #859 using a variable quickly with this selector ("le_resize")

idgserpro · 2019-10-08T19:24:23Z

@Mubra it's always good to put a closes #xxx in the description of a PR to create a link to the issue it fixing. Please add.

Mubra · 2019-10-08T19:33:17Z

ready @idgserpro

hvelarde

tests are failing; please review your solution.

hvelarde · 2019-10-08T21:24:28Z

CHANGES.rst

@@ -6,6 +6,12 @@ There's a frood who really knows where his towel is.
 2.2.1 (unreleased)
 ^^^^^^^^^^^^^^^^^^

+- The solution was to change the selector "$ le" to "le" because it did not take the charged element, only those created (fixes `#871 <https://github.com/collective/collective.cover/issues/871>`_).


these changelog entries are wrong; please fix them.

Entry must contain what has been fixed.

I don't know if the recent changes in "Travis" affect the tests, I corrected the error, adding these variables to the environment:
export ZSERVER_PORT = 55001
export LISTENER_HOST = 49999

With this, the robot tests run me without any problem, in fact I already did some simple tests of mine

rodfersou · 2019-10-08T21:50:34Z

well, use the $ in the variable name is a pattern used to know that the variable is a jquery object, and we have it in many projects, please keep this way.

rodfersou · 2019-10-08T21:55:51Z

webpack/app/js/layout.js

-    };
-    button.on('click', onClick.bind(this));
+      le_delete.trigger('modified.layout');
+    });


please use the bind this in the event instead to fix the this inside the jquery event. ex.:

https://github.com/collective/collective.cover/blob/master/webpack/app/js/cssclasswidget.js#L19

this way you don't need the le_delete variable.

also we are using ES6 here, prefer to use let and const instead of var to create variables; prefer to use arrow functions instead of old functions if possible.

please take a looks at http://es6-features.org

rodfersou · 2019-10-08T21:56:46Z

webpack/app/js/layout.js

@@ -478,7 +479,7 @@ export default class LayoutView {
      $('#slider').off("slide");
      $('#slider').on("slide", function(event, ui) {
        column.attr('data-column-size', ui.value);
-        le.trigger('modified.layout');
+        le_resize.trigger('modified.layout');


please look at the comments about le_delete, the same apply here.

"le_resize" is needed cause "le" is not reached into function

yes, that's why you should use the .bind(this) code when add an event, look at the other comments please.

rodfersou

Please look at my comments, need some changes to keep everything consistent in the codebase.

Mubra · 2019-10-08T22:12:47Z

Ok @rodfersou , reviewing, thanks for the comments 👍

Mubra · 2019-10-08T23:37:23Z

proof

idgserpro

Please create a new PR to try fix the tests. This has to work without the instance being started. These errors were most likely caused by the new version 4.3.19 of Plone.

It would also be good testing for this PR.

idgserpro · 2019-10-09T00:27:46Z

CHANGES.rst

@@ -6,6 +6,12 @@ There's a frood who really knows where his towel is.
 2.2.1 (unreleased)
 ^^^^^^^^^^^^^^^^^^

+- The solution was to change the selector "$ le" to "le" because it did not take the charged element, only those created (fixes `#871 <https://github.com/collective/collective.cover/issues/871>`_).


Entry must contain what has been fixed.

idgserpro · 2019-10-09T00:28:46Z

CHANGES.rst

+- The solution was to change the selector "$ le" to "le" because it did not take the charged element, only those created (fixes `#871 <https://github.com/collective/collective.cover/issues/871>`_).
+    [Mubra]
+
+  - This indirectly corrects issue `#859 <https://github.com/collective/collective.cover/issues/859>`_ using a variable quickly with this selector ("le_resize") (fixes `#859 <https://github.com/collective/collective.cover/issues/859>`_).


Entry must contain what has been fixed.

I still see console error while resizing a column:

cover-2a86fa7.js:1 Uncaught TypeError: Cannot read property 'trigger' of undefined at HTMLDivElement.<anonymous> (cover-2a86fa7.js:1) at HTMLDivElement.dispatch (++resource++plone.app.jquery.js:3) at HTMLDivElement.i (++resource++plone.app.jquery.js:3) at Object.trigger (++resource++plone.app.jquery.js:3) at HTMLDivElement.<anonymous> (++resource++plone.app.jquery.js:3) at Function.each (++resource++plone.app.jquery.js:2) at init.each (++resource++plone.app.jquery.js:2) at init.trigger (++resource++plone.app.jquery.js:3) at e.<computed>.<computed>._trigger (collective.js.jqueryui.custom.min.js:16) at e.<computed>.<computed>._slide (collective.js.jqueryui.custom.min.js:131)

I found the problem, if I need to declare "let le_resize = this.$le;". @rodfersou's comment is correct (that's why I had deleted this variable), but using a variable "le_resize" I think, helps to understand that it is a local object, if you add this, the console error should no longer appear.

I don't know if the recent changes in "Travis" affect the tests, I corrected the error, adding these variables to the environment:
export ZSERVER_PORT = 55001
export LISTENER_HOST = 49999

With this, the robot tests run me without any problem, in fact I already did some simple tests of mine

Mubra · 2019-10-09T16:57:23Z

I will close this PR to generate a new one with the correct changes, also to verify the "travis" tests.
@hvelarde
@idgserpro
@rodfersou
@scmos

fix issue 871

8f6dd41

hvelarde requested changes Oct 8, 2019

View reviewed changes

hvelarde requested review from rodfersou and idgserpro October 8, 2019 21:41

rodfersou reviewed Oct 8, 2019

View reviewed changes

rodfersou requested changes Oct 8, 2019

View reviewed changes

repair: jquery object le for

2a86fa7

idgserpro requested changes Oct 9, 2019

View reviewed changes

Mubra closed this Oct 9, 2019

idgserpro deleted the fix_issue_859 branch October 9, 2019 22:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Solve issue 859 and 871 #872

Solve issue 859 and 871 #872

Mubra commented Oct 8, 2019 •

edited

Loading

idgserpro commented Oct 8, 2019

Mubra commented Oct 8, 2019

hvelarde left a comment

hvelarde Oct 8, 2019

idgserpro Oct 9, 2019

Mubra Oct 9, 2019

rodfersou commented Oct 8, 2019

rodfersou Oct 8, 2019

rodfersou Oct 8, 2019

scmos Oct 8, 2019

rodfersou Oct 9, 2019

rodfersou left a comment

Mubra commented Oct 8, 2019

Mubra commented Oct 8, 2019

idgserpro left a comment

idgserpro Oct 9, 2019

idgserpro Oct 9, 2019

idgserpro Oct 9, 2019

Mubra Oct 9, 2019

Mubra Oct 9, 2019

Mubra commented Oct 9, 2019

Solve issue 859 and 871 #872

Solve issue 859 and 871 #872

Conversation

Mubra commented Oct 8, 2019 • edited Loading

idgserpro commented Oct 8, 2019

Mubra commented Oct 8, 2019

hvelarde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodfersou commented Oct 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodfersou left a comment

Choose a reason for hiding this comment

Mubra commented Oct 8, 2019

Mubra commented Oct 8, 2019

idgserpro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mubra commented Oct 9, 2019

Mubra commented Oct 8, 2019 •

edited

Loading