Refactor js classes #96
Conversation
Debatty-Tom
force-pushed
the
refactor-js-classes
branch
from
a187605 to
fe7d44c
Compare
resources/js/Modal.js
Outdated
| return; | ||
| } | ||
|
|
||
| this.getValues(); |
There was a problem hiding this comment.
Please rename to this.getUIElements() instead of this.getValues(). Ideally, all DOM elements are stored in the same this.elements object, such as:
this.elements = this.getUIElements(document.querySelector('#cookies-policy'));
if(! this.elements) {
return;
}This elements object can then be further used like this: this.elements.root, this.elements.reset, ...
The current this.text attribute should be renamed to this.translations and parsed after this.elements has been set, such as:
this.translations = this.getUITranslations();getUITranslations() {
let value = this.elements.root.getAttribute('data-text');
this.elements.root.removeAttribute('data-text');
return JSON.parse(value);
}There was a problem hiding this comment.
Update: maybe getUITranslations will not be necessary, take a look at this comment below.
resources/views/cookies.blade.php
Outdated
| @if(!$isReset) | ||
| <script data-cookie-consent> | ||
| let element = document.querySelector('#cookies-policy'); | ||
| if (element) { | ||
| element.classList.add('cookies--pre-init'); | ||
| element.classList.add('cookies--closing'); | ||
| } | ||
| </script> | ||
| @endif |
There was a problem hiding this comment.
Don't like this at all. Should be handled inside the modal.js init method. In all cases, the #cookies-policy element should be hidden when JS is enabled and the modal's script has not been initiated.
src/CookiesManager.php
Outdated
| return view('cookie-consent::cookies', [ | ||
| 'cookies' => $this->registrar, | ||
| 'policy' => $policy, | ||
| 'isReset' => $isReset, |
There was a problem hiding this comment.
Instead of providing a isReset attribute, it would be better to provide a RenderingContext (Enum) value to the view. This way developers can hook in all the possible modal rendering contexts:
enum CookieConsentState
{
// User has yet to configure consent
case Pending;
// User has defined a consent configuration
case Configured;
// User has configured consent but wants to show the modal in order to eventually make changes
case Updating;
// User had configured consent but chose to reset the settings (similar to "pending")
case Reset;
}There was a problem hiding this comment.
After reviewing other open PRs, this would be more related to PR #99. Please add this enum there.
In this PR's contect, I don't think isReset is still needed here because of this comment.
| public function __invoke(Request $request) | ||
| { | ||
|
|
||
| } |
There was a problem hiding this comment.
Since both routes leading to this controller have named methods, the __invoke method can be removed.
| public function getModalScript() | ||
| { | ||
| $content = file_get_contents(LCC_ROOT . '/dist/modal.js'); | ||
| return response($content)->header('Content-Type', 'application/javascript'); | ||
| } |
There was a problem hiding this comment.
Maybe use this opportunity to pass the translation strings to the modal's constructor, like we did for the API routes configuration in getCookiesScript() ?
…t the reset button is always clearly identifiable.
…nly for the modal handling
…latency and only pass the cookie script inside the head tag
2 participants
Cover #26, #28, and PR #89 which solve #47 and #63. Transitions on the cookie modal don’t work without JavaScript.
Refactor script.js file by creating a modal object. Each logic is now in a dedicated file and we make sure document is fully loaded before doing everything.