Convert jQuery .on event listeners to their native JS equivalents #1035
Conversation
|
I have completed testing on these browsers
|
|
You should see and "Update branch" button below that can merge the latest changes from main into this branch automatically. |
- `event.preventDefault()` added for more buttons - moved `prefixElement` up in the code - semicolons added
|
I have made all the changes requested by you before we close this PR I want to ask one question in general about contributing to open source. A lot of the time I get confused about whether to make a change in the codebase or not
Your guidance will help me improve my further contributions! Thank you! 😊🚀" |
|
Thank you, @RG7279805. I think in this case, the changes suggested were about minimizing the code and being more efficient, and they are directly relevant to the PR. The general rule of thumb is not to change things that are not relevant to the PR unless they are minor/small improvements in the code you are working on. But you are free to do the implementation that you consider to be the best and most concise/efficient, and the reviewer will of course tell you if you have made an irrelevant or unwanted change. If in doubt, you can of course make the suggestion in a comment and seek guidance. Of your options, I would say go for the first if you feel confident that it is relevant to the PR and is an improvement. Go for the second if you are in doubt. I don't think you should do the third option, because opening a new branch is not a good idea when you're working on a PR. You can always revert a commit to your existing branch/PR if necessary. It's much easier for a reviewer to follow a single branch/PR rather than having to jump to a new one and have to start reviewing all over again. I see the e2e tests (which include a test on IE11) are working well with your PR. I'll proceed to do some manual tests to double-check your tests. Thank you for testing. I'll get back to you asap, as it is probably ready to merge soon. |
There was a problem hiding this comment.
@RG7279805 Unfortunately, there is a problem: the code for switching between jQuery and Service Worker mode is not working. In Internet Explorer, it should give a warning that SW mode is unavailable. In Chrome/Edge, switching to jQuery mode does nothing (you can see that the API panel at the bottom does not change as it should do
It seems that the change event is not properly registered for the controls. Could you take a look and debug?
|
This is the message you should see when attempting to switch to SW mode in IE11 or IE Mode (Edge). I've just checked, and it's working fine on main, so it is definitely an issue with the jQuery conversion on this branch. NB It's not just IE11 that is affected, switching is also not working in modern browsers.
|
|
Also, remember that after making changes, you need to rebuild the dist directory before testing on IE11, using |
|
I am sorry for this mistake when I clicked on the radio SW radio was selected so i thought its working (should have tested the original behaviour) nonetheless I have applied the fix 😥 I have completed testing on these browsers
|
|
OK, thanks @RG7279805. If you could update the branch again, I have incorporated a specific e2e test for the jQuery and ServiceWorker modes. It's not an in-depth test of functionality, but it could pick up this kind of issue. I would be interested to see how the test performs on your branch. In the meantime, I'll continue my manual testing. |
|
Let me know if you're happy for this to be merged now. |
|
Sure @Jaifroid, And Thanks again for being my mentor |
|
You're welcome, and thank you for the very useful PR. |
closes #918