Fix undefined methods 'addClass' and `removeClass` on a PrototypeJS Element #22560

markvds · 2019-04-30T15:15:57Z

Description

This problem was caused by 5a283e3 and reported in #20843. The mentioned commit adds two lines in js.phtml with non existing methods: panel.addClass(activePanelClass); and panel.removeClass(activePanelClass);. Both methods do not exist in PrototypeJS and should have been addClassName() and removeClassName() respectively.

The incorrect method names cause a Javascript error 'Uncaught TypeError: panel.addClass is not a function' in the console and prevents the Edit Attribute form from saving.

Enabling the Swatches module, as mentioned in the issue, only solves the issue because it replaces the phtml file with its own file.

Fixed Issues (if relevant)

Uncaught TypeError: panel.addClass is not a function when Swatches are disabled #20843: Uncaught TypeError: panel.addClass is not a function when Swatches are disabled

Manual testing scenarios

Disable Swatches modules (otherwise, the mentioned phtml file will not be used)
In admin, go to Stores > Attributes > Product
Click any attribute
Try to save the attribute
Nothing happens without the code change, but the attribute saves fine with the code change

Contribution checklist

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

m2-assistant · 2019-04-30T15:16:00Z

Hi @markvds. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento-engcom-team give me test instance - deploy test instance based on PR changes
@magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

magento-cicd2 · 2019-04-30T15:16:00Z

All committers have signed the CLA.

orlangur

@markvds please use jQuery equivalents instead.

After changes are applied and all builds are green, please squash them into a single commit so that we have perfectly clean history 😉

markvds · 2019-04-30T15:51:49Z

@orlangur I cannot imagine that it makes the situation any better; it's all surrounded by PrototypeJS code. I think it would even make readibility worse.

orlangur · 2019-04-30T15:53:49Z

@markvds we don't need to add any new PrototypeJS-related code. Feel free to refactor any additional occurrences in file with jQuery.

barryvdh · 2019-05-01T08:41:23Z

@markvds Thank you for your PR.

@orlangur I would keep the bugfix PR as minimal as possible to make it clear what the intent and actual fix of this PR is. Also to make it easier to avoid merge conflicts etc.Refactoring the prototype should happen in a seperate PR imho.

orlangur · 2019-05-01T13:27:10Z

@barryvdh it will be minimal as I suggested since jQuery is already injected.

@markvds please apply necessary changes.

jissereitsma · 2019-05-01T17:28:59Z

My 50 cents: The proposed fix of @markvds is a bugfix. However, the suggestion of @orlangur would lead to a code enhancement. They are different things. I'm not sure if postponing the bugfix is a good idea, waiting for the code enhancement. On the other hand, I'm not sure how much work the refactoring the jQuery code would be. Just my 50 cents.

orlangur

Initial code was jQuery-style. There is no need to step back. Please replace all addClassName/removeClassName.

orlangur · 2019-05-01T22:03:39Z

app/code/Magento/Catalog/view/adminhtml/templates/catalog/product/attribute/js.phtml

@@ -45,11 +45,11 @@ function checkOptionsPanelVisibility(){

        if($('frontend_input') && ($('frontend_input').value=='select' || $('frontend_input').value=='multiselect')){
            panel.show();
-            panel.addClass(activePanelClass);
+            panel.addClassName(activePanelClass);


Suggested change

panel.addClassName(activePanelClass);

jQuery(panel).addClass(activePanelClass);

orlangur

After changes are applied and all builds are green, please squash them into a single commit so that we have perfectly clean history 😉

jissereitsma · 2019-05-02T05:30:54Z

@orlangur You are awesome

…lement The code was originally copied from the Swatches module, which already uses jQuery. In this file however, the 'panel' variable is not a jQuery object but a DOM Element.

markvds · 2019-05-02T10:01:14Z

I changed the PR.

@orlangur I finally get your point. You basically say the method name was correct, but the context was wrong. I assumed the context was right but the method name was wrong. It's just a different view on the situation. Understanding your thoughts helps me in future PRs. Thanks for explaining!

orlangur · 2019-05-02T10:20:37Z

@markvds yep, sorry for confusion, will try to formulate better next time 🤝

magento-engcom-team · 2019-05-02T10:21:35Z

Hi @orlangur, thank you for the review.
ENGCOM-5020 has been created to process this Pull Request

magento-engcom-team · 2019-05-02T10:21:36Z

@markvds thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

soleksii · 2019-05-03T12:51:44Z

✔️ QA Passed

Before:

After:

…PrototypeJS Element magento#22560

m2-assistant · 2019-05-10T22:17:06Z

Hi @markvds, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

See: magento/magento2#22560

magento-engcom-team added Area: Frontend Component: Catalog Release Line: 2.3 labels Apr 30, 2019

orlangur requested changes Apr 30, 2019

View reviewed changes

ghost assigned orlangur Apr 30, 2019

ghost added the Progress: needs update label Apr 30, 2019

orlangur requested changes May 1, 2019

View reviewed changes

Fix undefined methods 'addClass' and 'removeClass' on a PrototypeJS E…

1abadb5

…lement The code was originally copied from the Swatches module, which already uses jQuery. In this file however, the 'panel' variable is not a jQuery object but a DOM Element.

markvds force-pushed the pr-attribute-edit-addclassname branch from e5e7254 to 1abadb5 Compare May 2, 2019 09:53

orlangur approved these changes May 2, 2019

View reviewed changes

ghost added Progress: ready for testing and removed Progress: needs update labels May 2, 2019

soleksii self-assigned this May 3, 2019

ghost added Progress: testing in progress and removed Progress: ready for testing labels May 3, 2019

nmalevanec self-assigned this May 3, 2019

ghost added Progress: extended testing Progress: accept and removed Progress: testing in progress Progress: extended testing labels May 3, 2019

magento-engcom-team added the Award: bug fix label May 6, 2019

magento-engcom-team mentioned this pull request May 10, 2019

Uncaught TypeError: panel.addClass is not a function when Swatches are disabled #20843

Closed

magento-engcom-team pushed a commit to okorshenko/magento2 that referenced this pull request May 10, 2019

ENGCOM-5020: Fix undefined methods 'addClass' and removeClass on a …

7da8196

…PrototypeJS Element magento#22560

magento-engcom-team merged commit 1abadb5 into magento:2.3-develop May 10, 2019

magento-engcom-team added this to the Release: 2.3.3 milestone May 10, 2019

textarea mentioned this pull request Jun 5, 2019

"panel.addClass is not a function" error in admin product attribute edit page #23117

Closed

mzanibelli pushed a commit to occitech/elasticsuite that referenced this pull request Nov 5, 2019

Backport ENGCOM-5020: Fix undefined methods addClass and removeClass

dbf01b6

See: magento/magento2#22560

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix undefined methods 'addClass' and `removeClass` on a PrototypeJS Element #22560

Fix undefined methods 'addClass' and `removeClass` on a PrototypeJS Element #22560

markvds commented Apr 30, 2019 •

edited by LexKoomen

Loading

m2-assistant bot commented Apr 30, 2019

magento-cicd2 commented Apr 30, 2019 •

edited

Loading

orlangur left a comment

markvds commented Apr 30, 2019

orlangur commented Apr 30, 2019

barryvdh commented May 1, 2019

orlangur commented May 1, 2019

jissereitsma commented May 1, 2019

orlangur left a comment

orlangur May 1, 2019

orlangur left a comment

jissereitsma commented May 2, 2019

markvds commented May 2, 2019

orlangur commented May 2, 2019

magento-engcom-team commented May 2, 2019

magento-engcom-team commented May 2, 2019

soleksii commented May 3, 2019

m2-assistant bot commented May 10, 2019

	panel.addClassName(activePanelClass);
	jQuery(panel).addClass(activePanelClass);

Fix undefined methods 'addClass' and removeClass on a PrototypeJS Element #22560

Fix undefined methods 'addClass' and removeClass on a PrototypeJS Element #22560

Conversation

markvds commented Apr 30, 2019 • edited by LexKoomen Loading

Description

Fixed Issues (if relevant)

Manual testing scenarios

Contribution checklist

m2-assistant bot commented Apr 30, 2019

magento-cicd2 commented Apr 30, 2019 • edited Loading

orlangur left a comment

Choose a reason for hiding this comment

markvds commented Apr 30, 2019

orlangur commented Apr 30, 2019

barryvdh commented May 1, 2019

orlangur commented May 1, 2019

jissereitsma commented May 1, 2019

orlangur left a comment

Choose a reason for hiding this comment

orlangur May 1, 2019

Choose a reason for hiding this comment

orlangur left a comment

Choose a reason for hiding this comment

jissereitsma commented May 2, 2019

markvds commented May 2, 2019

orlangur commented May 2, 2019

magento-engcom-team commented May 2, 2019

magento-engcom-team commented May 2, 2019

soleksii commented May 3, 2019

m2-assistant bot commented May 10, 2019

Fix undefined methods 'addClass' and `removeClass` on a PrototypeJS Element #22560

Fix undefined methods 'addClass' and `removeClass` on a PrototypeJS Element #22560

markvds commented Apr 30, 2019 •

edited by LexKoomen

Loading

magento-cicd2 commented Apr 30, 2019 •

edited

Loading