Use vanilla JS to avoid issues because of jquery versions #96
Conversation
Nakahiru
commented
Aug 7, 2020
•
Nakahiru
commented
| Questions | Answers |
|---|---|
| Description? | Use vanilla JS to avoid issues because of jquery versions |
| Type? | bug fix |
| BC breaks? | no |
| Deprecations? | no |
| Fixed ticket? | Fixes all issues listed in https://github.com/PrestaShopCorp/psgdpr/issues/103 when used in combination with PrestaShop/PrestaShop#20628 PR on Core |
| How to test? | See the scenario described in PrestaShop/PrestaShop#20605 |
In theory we should provide IDs for all possible "items a module might want to select", right ? (if you have a small list, it would be awesome !) |
|
@matks Yes but I think it is a bad practise to do what we have done for the past version. We should never execute some js stuff based on the Dom, no ? For x reasons if the Dom changed the js will not work anymore. |
In theory we try not to introduce BC breaks in the DOM because we know modules rely on it 😄 . For migration to Symfony it cannot be avoided, too much changes are happening. But we try to maintain upward compatibility for selectors. What replacement do you use for selectors ? If you dont use selectors, what else do you use ? |
|
Ok ok good to know. In this case, we do not use any kind of replacement. We decided to not hide addresses. We choose to do that in the past to avoid some potentially SAV. The rgpd module cannot delete these content from the database because it is used for accountancy purposes. Rgpd law allow to keep some data in this case. |
|
Otherwise, the module used these two selectors before: #addressShipping and #addressInvoice. These two selector are now missing on 1.7.7. |
…de in order to add selectors
|
With PrestaShop/PrestaShop#20628 the missing selectors are back 🎉 |
| addressInvoice.empty(); | ||
| addressInvoice.append('<p>'+psgdprNoAddresses+'</p>'); | ||
| addressInvoice.css("visibility", "visible"); | ||
| document.addEventListener('DOMContentLoaded', function() { |
There was a problem hiding this comment.
Why not using $(document).on('ready')
Most browsers provide similar functionality in the form of a DOMContentLoaded event. However, jQuery's .ready() method differs in an important and useful way: If the DOM becomes ready and the browser fires DOMContentLoaded before the code calls .ready( handler ), the function handler will still be executed. In contrast, a DOMContentLoaded event listener added after the event fires is never executed.
from the jQuery doc
There was a problem hiding this comment.
I think @V4LUX wanted to avoid using jQuery.
He did it because the current code does not work well with 1.7.7, and the bug comes from our update of jQuery version.
Since psgdpr module is supposed to work on multiple PS 1.7 versions, I think he chose a "no jQuery" solution to make sure it works on all PS versions.
|
To sum up : |
Hi, while exploring the issue with @V4LUX we actually identified 2 issues:
The information message:
Yes it's blocking. Issue 1 has been adressed on the Core in PR PrestaShop/PrestaShop#20628. The PR restores the missing selectors => the adresses are hidden. So we need the 2 PRs to be validated. The most important PR is the one on the Core because by hiding the adresses, we comply with GDPR rules, but the 2nd PR is useful too 😅 |
|
I tested on my mac, installed everything correctly with @sarahdib and found an error in console + the GPDR info not displayed: On Sarah's laptop, we were able to test: I also wonder if we shouldn't have only one time the message about GDPR? even thought it hides two things. |
ghost
mentioned this pull request
matks
changed the title
|
LGTM Thanks @V4LUX !! |
