-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Issue #30594 - added preventDefault if EventListner is added for a tag #30628
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
base: 2.4-develop
Are you sure you want to change the base?
Conversation
Hi @amitsamsukha. Thank you for your contribution
❗ Automated tests can be triggered manually with an appropriate comment:
You can find more information about the builds here ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review. For more details, please, review the Magento Contributor Guide documentation. 🕙 You can find the schedule on the Magento Community Calendar page. 📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket. 🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel ✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel |
@magento give me test instance |
Hi @amitsamsukha. Thank you for your request. I'm working on Magento instance for you. |
Hi @amitsamsukha, here is your Magento Instance: https://01b23675a576e47ab7afacff97f796dc.instances.magento-community.engineering |
@magento run all tests |
@magento run Functional Tests CE |
Can we have test 2 cases in one PR ? |
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.
Hi @amitsamsukha. Thank you for providing the fix. According to the definition of done all changes should be covered buy autotests.
Could I ask you to cover this fix with an MFTF test that will assert that after clicking the "Preview" link the parent page stays unchanged?
Thank you!
lib/internal/Magento/Framework/View/Helper/SecureHtmlRenderer.php
Outdated
Show resolved
Hide resolved
Hi @mrtuvn. I would suggest not to merge these two solutions. So, leave the "preventDefault" in this PR and other changes in the mentioned PR. The main reason for that - author of #29991 will have to create an additional test that covers email templates preview. So, it's better to have 2 PRs for 2 different issues. |
@magento run Functional Tests CE |
@magento run Functional Tests EE |
@magento run all tests |
@magento run all tests |
Hi @amitsamsukha. Would you take care of the test coverage, please? Please, refer to my comment above Thank you! |
@rogyar can you guide me on how to resolve the following issues : you can check the below link : |
@magento run Functional Tests CE |
@magento give me test instance |
Hi @amitsamsukha. Thank you for your request. I'm working on Magento instance for you. |
Hi @amitsamsukha, here is your Magento Instance: https://01b23675a576e47ab7afacff97f796dc.instances.magento-community.engineering |
@magento give me test instance |
Hi @amitsamsukha. Thank you for your request. I'm working on Magento instance for you. |
Hi @amitsamsukha, here is your Magento Instance: https://01b23675a576e47ab7afacff97f796dc.instances.magento-community.engineering |
@magento run all tests |
@magento run all tests |
Hi @amitsamsukha. There's nothing wrong with the existing tests at the moment. I was asking you to add an additional functional test that will emulate all the steps to reproduce the issue and make sure that the issue does not appear anymore after your fix. Thank you. |
} | ||
|
||
script; | ||
|
||
return $this->renderTag('script', ['type' => 'text/javascript'], $script, false); |
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.
Hard-code logic check nodename is not good way. IMO
You should add change method _toLinkHtml in https://github.com/magento/magento2/blob/2.4-develop/app/code/Magento/Backend/Block/Widget/Grid/Column/Renderer/Action.php#L143
CC: @rogyar @zaximus84
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.
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.
@mrtuvn
thanks for the suggestion, but the preventDefault only works if called within function onclick into:
the issue is resolved by just adding onclick if condition but I have put a double-check for the anchor tag.
let me know if I need to remove the anchor tag check in this.
CC: @rogyar @zaximus84
@magento run all tests |
Hi @amitsamsukha. Please, do not forget about the auto tests mentioned in the following comment. Thanks |
Description (*)
renderEventListenerAsTag function of class Magento\Framework\View\Helper\SecureHtmlRenderer
execute/apply the event for tag
so the default event must prevent in this case.
Related Pull Requests
Fixed Issues (if relevant)
Manual testing scenarios (*)
Questions or comments
Contribution checklist (*)