Skip to content
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

Psgdpr module checkbox fix #360

Merged
merged 3 commits into from
Dec 15, 2022

Conversation

JBLach
Copy link
Contributor

@JBLach JBLach commented Aug 8, 2022

Questions Answers
Description? I have fixed psgdpr module checkbox
Type? bug fix
BC breaks? no
Deprecations? no
Fixed ticket? Fixes NA.
How to test? See screenshots below
Possible impacts? Please indicate what parts of the software we need to check to make sure everything is alright.

Before

image

After

image
image

@NeOMakinG
Copy link
Contributor

Thanks @JBLach you're on fire these days 🔥

I have a question, do you see any way to avoid that override?

@JBLach
Copy link
Contributor Author

JBLach commented Aug 9, 2022

I have a question, do you see any way to avoid that override?

Hi @NeOMakinG
Does this override cause any problems? Alternatively we can overwrite the front.js file and add these classes using js, but i don't think it's better solution, maybe someone else has an idea?

@NeOMakinG
Copy link
Contributor

I have a question, do you see any way to avoid that override?

Hi @NeOMakinG Does this override cause any problems? Alternatively we can overwrite the front.js file and add these classes using js, but i don't think it's better solution, maybe someone else has an idea?

Adding an override means that if the module get updated, this override will probably not be updated in the future, it's always a bad idea in case we can avoid it, this is why I asked

@JBLach
Copy link
Contributor Author

JBLach commented Aug 9, 2022

I have a question, do you see any way to avoid that override?

Hi @NeOMakinG Does this override cause any problems? Alternatively we can overwrite the front.js file and add these classes using js, but i don't think it's better solution, maybe someone else has an idea?

Adding an override means that if the module get updated, this override will probably not be updated in the future, it's always a bad idea in case we can avoid it, this is why I asked

Sure, i understand. I will think about a solution tomorrow

@JBLach
Copy link
Contributor Author

JBLach commented Aug 10, 2022

Hi @NeOMakinG

We can try to create a block in main module and extends the file, only changing the content in the block. WDYT?

{extends file="modules/psgdpr/views/templates/hook/displayGDPRConsent.tpl"}
{block name='gdpr_checkbox'}
  <div id="gdpr_consent" class="mt-2 gdpr_module_{$psgdpr_id_module|escape:'htmlall':'UTF-8'}">
    <span class="form-check">
      <input id="psgdpr_consent_checkbox_{$psgdpr_id_module|escape:'htmlall':'UTF-8'}" name="psgdpr_consent_checkbox" type="checkbox" value="1" class="form-check-input psgdpr_consent_checkboxes_{$psgdpr_id_module|escape:'htmlall':'UTF-8'}">
      <label class="form-check-label psgdpr_consent_message">
        {$psgdpr_consent_message nofilter}{* html data *}
      </label>
    </span>
  </div>
{/block}

@NeOMakinG
Copy link
Contributor

Hi @NeOMakinG

We can try to create a block in main module and extends the file, only changing the content in the block. WDYT?

{extends file="modules/psgdpr/views/templates/hook/displayGDPRConsent.tpl"}
{block name='gdpr_checkbox'}
  <div id="gdpr_consent" class="mt-2 gdpr_module_{$psgdpr_id_module|escape:'htmlall':'UTF-8'}">
    <span class="form-check">
      <input id="psgdpr_consent_checkbox_{$psgdpr_id_module|escape:'htmlall':'UTF-8'}" name="psgdpr_consent_checkbox" type="checkbox" value="1" class="form-check-input psgdpr_consent_checkboxes_{$psgdpr_id_module|escape:'htmlall':'UTF-8'}">
      <label class="form-check-label psgdpr_consent_message">
        {$psgdpr_consent_message nofilter}{* html data *}
      </label>
    </span>
  </div>
{/block}

Does it work if the template comes from the module directly?

I guess it would be cleaner than overriding the whole file yes! I would agree.

@Hlavtox wdyt?

@JBLach
Copy link
Contributor Author

JBLach commented Aug 10, 2022

Hi @NeOMakinG
We can try to create a block in main module and extends the file, only changing the content in the block. WDYT?

{extends file="modules/psgdpr/views/templates/hook/displayGDPRConsent.tpl"}
{block name='gdpr_checkbox'}
  <div id="gdpr_consent" class="mt-2 gdpr_module_{$psgdpr_id_module|escape:'htmlall':'UTF-8'}">
    <span class="form-check">
      <input id="psgdpr_consent_checkbox_{$psgdpr_id_module|escape:'htmlall':'UTF-8'}" name="psgdpr_consent_checkbox" type="checkbox" value="1" class="form-check-input psgdpr_consent_checkboxes_{$psgdpr_id_module|escape:'htmlall':'UTF-8'}">
      <label class="form-check-label psgdpr_consent_message">
        {$psgdpr_consent_message nofilter}{* html data *}
      </label>
    </span>
  </div>
{/block}

Does it work if the template comes from the module directly?

I guess it would be cleaner than overriding the whole file yes! I would agree.

@Hlavtox wdyt?

I tested this solution and it works

@NeOMakinG
Copy link
Contributor

Hi @NeOMakinG
We can try to create a block in main module and extends the file, only changing the content in the block. WDYT?

{extends file="modules/psgdpr/views/templates/hook/displayGDPRConsent.tpl"}
{block name='gdpr_checkbox'}
  <div id="gdpr_consent" class="mt-2 gdpr_module_{$psgdpr_id_module|escape:'htmlall':'UTF-8'}">
    <span class="form-check">
      <input id="psgdpr_consent_checkbox_{$psgdpr_id_module|escape:'htmlall':'UTF-8'}" name="psgdpr_consent_checkbox" type="checkbox" value="1" class="form-check-input psgdpr_consent_checkboxes_{$psgdpr_id_module|escape:'htmlall':'UTF-8'}">
      <label class="form-check-label psgdpr_consent_message">
        {$psgdpr_consent_message nofilter}{* html data *}
      </label>
    </span>
  </div>
{/block}

Does it work if the template comes from the module directly?
I guess it would be cleaner than overriding the whole file yes! I would agree.
@Hlavtox wdyt?

I tested this solution and it works

Go for it! Seems way cleaner :)

Copy link
Contributor

@NeOMakinG NeOMakinG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So quick 🍬

@NeOMakinG
Copy link
Contributor

I guess we need towait for psgdpr to be released before merging this

Copy link
Contributor

@NeOMakinG NeOMakinG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting for psgdpr release

Copy link
Contributor

@kpodemski kpodemski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gdpr module has been released, we can merge this :)

@kpodemski kpodemski added this to the Beta milestone Dec 15, 2022
@kpodemski kpodemski merged commit 5552b91 into PrestaShop:develop Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants