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

Fix responsive CSS in add-to-cart modal #27502

Merged
merged 1 commit into from
Feb 1, 2022
Merged

Fix responsive CSS in add-to-cart modal #27502

merged 1 commit into from
Feb 1, 2022

Conversation

saulaski
Copy link
Contributor

Questions Answers
Branch? 1.7.8.x
Description? Mobile display was messed up in add-to-cart modal. Responsive margins are now fixed.
Type? bug fix
Category? FO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #26642
How to test?
  1. Open product page on mobile or dev tools responsive mode
  2. Add product to cart and wait for the modal to pop up
Possible impacts? None

This change is Reviewable

@saulaski saulaski requested a review from a team as a code owner January 27, 2022 21:45
@prestonBot prestonBot added 1.7.8.x Branch Bug fix Type: Bug fix labels Jan 27, 2022
@SharakPL
Copy link
Contributor

SharakPL commented Jan 28, 2022 •
edited
Loading

It should be done with flex one-liner. Media queries are unnecessary. Besides it doesn't fully fix #26642. Image and buttons still need fixing.
Also further changes are required and await maintainers' decision #26642 (comment)

@saulaski saulaski force-pushed the fix-responsive-css-in-add-to-cart-modal branch from b4a88d8 to 07d6672 Compare January 28, 2022 23:31
@saulaski
Copy link
Contributor Author

saulaski commented Jan 28, 2022 •
edited
Loading

@prestascott @SharakPL I assumed the issue was only a minor bug fix. But I've just implemented the mockup proposed in issue #26642.
Sadly the Bootstrap 3 standard grid cannot implement the flex solution.

Desktop screenshot

desktop

Mobile screenshot

mobile

@saulaski saulaski force-pushed the fix-responsive-css-in-add-to-cart-modal branch from 07d6672 to 9e86908 Compare January 28, 2022 23:54
@SharakPL
Copy link
Contributor

My mockup hasn't been accepted yet.
Just because BS3 can't handle flex by itself doesn't mean you can't use it 😉

@saulaski saulaski force-pushed the fix-responsive-css-in-add-to-cart-modal branch from 9e86908 to c17ac2a Compare January 31, 2022 11:23
@saulaski
Copy link
Contributor Author

saulaski commented Jan 31, 2022 •
edited
Loading

@SharakPL Here we should split the bug fix and the layout improvement in 2 distinct PRs:

  • Bug fix is indeed required in patch version 1.7.8.x
  • Layout improvement is only required in new major version 8, to avoid BC issues

Quick fix is available in current PR, with the result shown below:
mobile-178x

Then I'm gonna refactore the entire modal layout in full Flex in a distinct PR dedicated to version 8.

matks
matks approved these changes Jan 31, 2022
View reviewed changes
Copy link
Contributor

@matks matks left a comment

Choose a reason for hiding this comment

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

Seems OK for me

@NeOMakinG NeOMakinG added the Waiting for QA Status: action required, waiting for test feedback label Feb 1, 2022
@HanaRebaiQA HanaRebaiQA self-assigned this Feb 1, 2022
HanaRebaiQA
HanaRebaiQA approved these changes Feb 1, 2022
View reviewed changes
Copy link

@HanaRebaiQA HanaRebaiQA left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hello @saulaski

I have tested this PR. The issue is fixed.
I checked with multistore and different languages on mobile and responsive mode.
image
image
image

https://watch.screencastify.com/v/X4v7dNyfa3kqJaOeNCaA
https://watch.screencastify.com/v/xGyo3leNDFbEmyEIcOJQ

So, QA ✔️

Thanks!

@HanaRebaiQA HanaRebaiQA added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Feb 1, 2022
@prestonBot
Copy link
Collaborator

QA approved, well done! Message to the maintainers: do not forget to milestone it before the merge.

@Progi1984 Progi1984 added this to the 1.7.8.4 milestone Feb 1, 2022
@Progi1984 Progi1984 linked an issue Feb 1, 2022 that may be closed by this pull request
[FO] Mobile add-to-cart modal is messed up on 1.7.8.x - regression #26642
Closed
2 tasks
@Progi1984 Progi1984 merged commit 372100f into PrestaShop:1.7.8.x Feb 1, 2022
@Progi1984
Copy link
Member

Thanks @saulaski & @HanaRebaiQA

@Progi1984 Progi1984 mentioned this pull request Feb 1, 2022
Closed
2 tasks
@MatShir MatShir mentioned this pull request Apr 6, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matks matks matks approved these changes

@NeOMakinG NeOMakinG NeOMakinG approved these changes

@HanaRebaiQA HanaRebaiQA HanaRebaiQA approved these changes

Assignees

@HanaRebaiQA HanaRebaiQA

Labels
1.7.8.x Branch Bug fix Type: Bug fix QA ✔️ Status: check done, code approved
Projects
None yet
Milestone
1.7.8.4
Development

Successfully merging this pull request may close these issues.

[FO] Mobile add-to-cart modal is messed up on 1.7.8.x - regression
7 participants
@saulaski @SharakPL @prestonBot @Progi1984 @matks @NeOMakinG @HanaRebaiQA