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

Fix wrong templates used on product page columns #155

Merged
merged 4 commits into from Jan 17, 2023

Conversation

jsuzineau
Copy link
Contributor

@jsuzineau jsuzineau commented Oct 21, 2022

Questions Answers
Description? This fix will make correct module templates to be used in product columns.
Type? bug fix
BC breaks? no
Deprecations? no
Fixed ticket? Fixes PrestaShop/PrestaShop#30802
How to test? Prestashop 1.7.8, Theme Classic 1.0.0, ps_linklist 5.0.5.
Check FO product page

…rendered with the column template.

I added a test on displayLeftColumnProduct to solve this.
Copy link
Contributor

@SharakPL SharakPL left a comment

Choose a reason for hiding this comment

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

Hi @jsuzineau.
Thanks for the PR and sorry for the delay.

Small improvements needed and we're good to go :)

BTW could you also make a small refactor on line 146 and move lonely ; to the end of line 145?

ps_linklist/ps_linklist.php

Lines 142 to 146 in 564b2b5

$dataLoadedWithSuccess = $dataLoadedWithSuccess
&& $this->registerHook('displayFooter')
&& $this->registerHook('actionUpdateLangAfter')
&& $this->registerHook('actionGeneralPageSave')
;

It should look like this:

        $dataLoadedWithSuccess = $dataLoadedWithSuccess
            && $this->registerHook('displayFooter')
            && $this->registerHook('actionUpdateLangAfter')
            && $this->registerHook('actionGeneralPageSave');

ps_linklist.php Outdated Show resolved Hide resolved
@SharakPL SharakPL changed the title When transplanted on left column of product page, this module wasn't … Fix wrong templates used on product page columns Jan 10, 2023
@SharakPL SharakPL added the Waiting for author Waiting for author feedback label Jan 12, 2023
Hlavtox
Hlavtox previously approved these changes Jan 12, 2023
Hlavtox
Hlavtox previously approved these changes Jan 12, 2023
SharakPL
SharakPL previously approved these changes Jan 12, 2023
@SharakPL SharakPL dismissed stale reviews from Hlavtox and themself via 9bad1bb January 12, 2023 19:13
@SharakPL SharakPL added Waiting for QA and removed Waiting for author Waiting for author feedback labels Jan 12, 2023
@jsuzineau
Copy link
Contributor Author

jsuzineau commented Jan 14, 2023 via email

@Hlavtox
Copy link
Contributor

Hlavtox commented Jan 14, 2023

@jsuzineau All is good now. Now a QA tester will test it. If everything is fine, he will put "QA ✅" label and we will merge it. 👍

@SharakPL
Copy link
Contributor

Hi @jsuzineau. I noticed you're not really active on GitHub appart from this single PR. I wasn't sure if you'll be able to finish it so I did refactoring myself. If you want to update your repo locally just use git pull command, but you don't need to since the work here is over. Now we only await final approval from QA team to be able to merge it into the main repo. After that your branch process_displayLeftColumnProduct won't be needed anymore and you'll be free to delete it.

Thanks for pointing out the problem with the column templates.

@MhiriFaten MhiriFaten self-assigned this Jan 16, 2023
Copy link

@MhiriFaten MhiriFaten left a comment

Choose a reason for hiding this comment

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

Hello @jsuzineau ,

I have checked your PR and the wrong templates were used on product page columns are fixed, but the touchspin button is misplaced !

image

Could you check it please?

Waiting for your feedback !
Thank you 😄

@MhiriFaten MhiriFaten added Waiting for author Waiting for author feedback and removed Waiting for QA labels Jan 16, 2023
@MhiriFaten MhiriFaten removed their assignment Jan 16, 2023
@SharakPL
Copy link
Contributor

Thanks @MhiriFaten for your feedback.
The bug with the quantity form needs to be fixed in another PR in classic theme repo. Since you confirmed the bug with the module templates is fixed by this PR I'll let myself to add proper QA label and merge it.

@SharakPL SharakPL added QA ✔️ Status: QA-Approved and removed Waiting for author Waiting for author feedback labels Jan 17, 2023
@SharakPL SharakPL added this to the 5.0.6 milestone Jan 17, 2023
@SharakPL SharakPL merged commit a54e0cf into PrestaShop:dev Jan 17, 2023
@Hlavtox
Copy link
Contributor

Hlavtox commented Jan 17, 2023

@MhiriFaten The behavior is because of two columns together, which is so old school that nobody tests it. :-) The same behavior is without this module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
QA ✔️ Status: QA-Approved
Projects
None yet
4 participants