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.

Sign up for GitHub

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

Jump to bottom

Add selectors mapping so themes can override it #20002

Merged
merged 8 commits into from Jul 22, 2020
Merged

Add selectors mapping so themes can override it #20002

merged 8 commits into from Jul 22, 2020

Conversation

NeOMakinG
Copy link

@NeOMakinG NeOMakinG commented Jun 30, 2020 •
edited by eternoendless

Questions Answers
Branch? develop
Description? This PR aim to give some extend abilities to theme developers by mapping JS selectors inside prestashop.selectors object, so they can easily change the markup of their theme and adapt the JS to their new markup
Type? improvement
Category? FO
BC breaks? no
Deprecations? no
How to test? The FO should continue to work as expected before this PR

This change is Reviewable

@NeOMakinG NeOMakinG requested a review from a team as a code owner June 30, 2020 14:43
@prestonBot prestonBot added develop Branch Improvement Type: Improvement labels Jun 30, 2020
@NeOMakinG NeOMakinG closed this Jul 1, 2020
@NeOMakinG NeOMakinG reopened this Jul 1, 2020
atomiix
atomiix previously approved these changes Jul 1, 2020
View reviewed changes
Copy link
Contributor

@atomiix atomiix left a comment

Choose a reason for hiding this comment

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

I'm not sure about the file package-lock.json being changed but I guess it's optional

@NeOMakinG NeOMakinG added the WIP Status: Work In Progress label Jul 1, 2020
@NeOMakinG
Copy link
Author

You're right I'll kick it

@NeOMakinG
Copy link
Author

I did checkout package-lock from upstream/develop, should be fine like this

@NeOMakinG NeOMakinG changed the title [WIP] Add selectors mapping so themes can override it Add selectors mapping so themes can override it Jul 2, 2020
@NeOMakinG NeOMakinG removed the WIP Status: Work In Progress label Jul 2, 2020
@NeOMakinG NeOMakinG mentioned this pull request Jul 2, 2020
Merged
@NeOMakinG NeOMakinG added the WIP Status: Work In Progress label Jul 2, 2020
@NeOMakinG NeOMakinG changed the title Add selectors mapping so themes can override it [WIP] Add selectors mapping so themes can override it Jul 2, 2020
@NeOMakinG
Copy link
Author

Added WIP because I need 1.7.6.x and 1.7.7.x to be merged on develop in order to finish

@NeOMakinG NeOMakinG removed the WIP Status: Work In Progress label Jul 16, 2020
@NeOMakinG NeOMakinG changed the title [WIP] Add selectors mapping so themes can override it Add selectors mapping so themes can override it Jul 16, 2020
@NeOMakinG NeOMakinG requested a review from a team July 16, 2020 14:23
@Progi1984 Progi1984 added this to the 1.7.8.0 milestone Jul 17, 2020
@Progi1984 Progi1984 added this to To be reviewed in PrestaShop 1.7.8.0 Jul 17, 2020
Progi1984
Progi1984 reviewed Jul 17, 2020
View reviewed changes
themes/_core/js/checkout-payment.js
Comment on lines +30 to +35
this.confirmationSelector = prestashop.selectors.checkout.confirmationSelector;
this.conditionsSelector = prestashop.selectors.checkout.conditionsSelector;
this.conditionAlertSelector = prestashop.selectors.checkout.conditionAlertSelector;
this.additionalInformatonSelector = prestashop.selectors.checkout.additionalInformatonSelector;
this.optionsForm = prestashop.selectors.checkout.optionsForm;
this.termsCheckboxSelector = prestashop.selectors.checkout.termsCheckboxSelector;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not directly use prestashop.selectors rather assign it to an another value ?

Copy link
Author

@NeOMakinG NeOMakinG Jul 17, 2020 •
edited

Choose a reason for hiding this comment

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

It could be a breaking change and this PR doesn't aim a complete refacto of FO

Progi1984
Progi1984 reviewed Jul 17, 2020
View reviewed changes
themes/_core/js/product.js
Comment on lines +56 to +60
prestashop !== null &&
prestashop.urls !== null &&
prestashop.urls.pages !== null &&
prestashop.urls.pages.product !== '' &&
prestashop.urls.pages.product !== null
Copy link
Contributor

Choose a reason for hiding this comment

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

&& at the start ;)

Copy link
Author

Choose a reason for hiding this comment

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

There are no linters on this part until #20080, shouldn't be blocking

Progi1984
Progi1984 reviewed Jul 17, 2020
View reviewed changes
Copy link
Contributor

@Progi1984 Progi1984 left a comment

Choose a reason for hiding this comment

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

2️⃣ Comments ;)

@NeOMakinG NeOMakinG requested a review from Progi1984 July 17, 2020 07:36
@matthieu-rolland matthieu-rolland added the Waiting for QA Status: action required, waiting for test feedback label Jul 20, 2020
@sarahdib sarahdib moved this from To be reviewed to To be merged in PrestaShop 1.7.8.0 Jul 22, 2020
@sarahdib sarahdib added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Jul 22, 2020
@Progi1984 Progi1984 merged commit f3ea038 into PrestaShop:develop Jul 22, 2020
@Progi1984
Copy link
Contributor

Thanks @NeOMakinG & @sarahdib

@Progi1984 Progi1984 moved this from To be merged to Done in PrestaShop 1.7.8.0 Jul 22, 2020
@prestashop-issue-bot prestashop-issue-bot bot added the Fixed Resolution: issue closed because fixed label Jul 22, 2020
@matks matks added Key feature Notable feature to be highlighted Developer Feature Developer-oriented feature labels Jul 30, 2020
@matks matks mentioned this pull request Jun 1, 2021
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PierreRambaud PierreRambaud PierreRambaud approved these changes

@matthieu-rolland matthieu-rolland matthieu-rolland approved these changes

@atomiix atomiix atomiix left review comments

@Progi1984 Progi1984 Awaiting requested review from Progi1984

Assignees
No one assigned
Labels
develop Branch Developer Feature Developer-oriented feature Fixed Resolution: issue closed because fixed Improvement Type: Improvement Key feature Notable feature to be highlighted QA ✔️ Status: check done, code approved
Projects
No open projects
PrestaShop 1.7.8.0
  
Done
Milestone
1.7.8.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@NeOMakinG @Progi1984 @PierreRambaud @matthieu-rolland @atomiix @sarahdib @matks @prestonBot