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

Remove harmful modal styles and fix product buttons #239

Closed
wants to merge 6 commits into from

Conversation

SharakPL
Copy link
Contributor

@SharakPL SharakPL commented Feb 1, 2024

Questions Answers
Description? These styles caused modals to be always active on the page, just invisible. This blocked elements behind modals from interaction.

Product buttons in wishlists didn't get proper styling, because of invalid checks.
Type? bug fix
BC breaks? no
Deprecations? no
Fixed ticket? PrestaShop/hummingbird#594 PrestaShop/PrestaShop#35276
Sponsor company -
How to test? 1. Install Hummingbird theme
2. Sign in and add few products to your wishlists
3. Go to My Account > My wishlists

Check that elements in the center of the browser screen can't be accessed because invisible modal is hanging there:

obraz

There are already styles for modals so inline vue overrides are useless here

.wishlist-modal {
display: none;
opacity: 0;
pointer-events: none;
z-index: 0;
&.show {
display: block;
opacity: 1;
pointer-events: all;
z-index: 1051;


As for the buttons, these checks simply don't work and are always false because product.customizable is an INT, not a STRING:

:class="{
'btn-secondary': product.customizable === '1',
'btn-primary': product.customizable === '0'
}"

Examples for Classic and Hummingbird themes:

Before

obraz
obraz

After

obraz
obraz

@SharakPL SharakPL changed the title Remove harmful modal styles Remove harmful modal styles and fix product buttons Feb 1, 2024
@leemyongpakva
Copy link

leemyongpakva commented Feb 15, 2024

@SharakPL I made a related issue at PrestaShop/hummingbird#594.
For the second issue, after applying your change, all products in Wishlist become Customize in both classic and hummingbird themes :(

@SharakPL
Copy link
Contributor Author

@leemyongpakva are you certain that you implemented all the changes correctly?

blockwishlist.mp4

@SharakPL SharakPL requested a review from a team February 15, 2024 11:27
@leemyongpakva
Copy link

@SharakPL I tested with these steps
rm -rf blockwishlist
git clone https://github.com/PrestaShop/blockwishlist.git
gh pr checkout 239
cd blockwishlist
npm run build
BO clear PS cache & FO clear browser cache
After PS: the first issue is solved but I encounter the second issue in a different style: all products are Customize.
Before PR: I don't see the second issue.

@SharakPL
Copy link
Contributor Author

Maybe all the products on your wishlist are customizable 🤔

@leemyongpakva
Copy link

leemyongpakva commented Feb 16, 2024

@SharakPL No, I have only 1 customizable product on my test site, the rest are not.

@SharakPL
Copy link
Contributor Author

rm -rf blockwishlist
git clone https://github.com/PrestaShop/blockwishlist.git
gh pr checkout 239
cd blockwishlist
npm run build

Wrong steps @leemyongpakva ! You did pr checkout inside modules instead of modules/blockwishlist so you probably checked out PrestaShop/PrestaShop#239 😉

Try like this:

  1. cd modules
  2. rm -rf blockwishlist
  3. git clone https://github.com/PrestaShop/blockwishlist.git
  4. cd blockwishlist
  5. gh pr checkout 239
  6. npm i *
  7. npm run build *
  • 6-7 are obsolete really since all assets are pre-built in this module so PR already includes everything

before

obraz

after

obraz

@leemyongpakva
Copy link

@SharakPL You're right about Step 4 @ Step 5 order 👍 By the way, with these correct steps, all Product buttons are still Customize (FYI, I use sample data of a fresh PS 8.1 install and there is only one Customizable product - Customizable mug).
I think creating a separate issue for Product buttons in wishlists didn't get proper styling, because of invalid checks will be better. That way, QA team can confirm the issue exist or not (I even don't see the issue with Product buttons from my side :)

@SharakPL
Copy link
Contributor Author

OK let me check it out. I only tested it on develop branch.

@SharakPL
Copy link
Contributor Author

@leemyongpakva I checked branch 8.1.x (basically 8.1.5 soon to be released) on php 8.1 and Classic theme v.2.1.3

before PR

obraz

after PR

obraz

So it's all good! I really have no idea why would you get all buttons labeled Customize

@Progi1984 Progi1984 closed this Feb 27, 2024
@Progi1984 Progi1984 reopened this Feb 27, 2024
@leemyongpakva
Copy link

@SharakPL I see your view. We need a third view from QA. If they can reproduce the second issue then we can ignore my different view ;)

@SharakPL
Copy link
Contributor Author

I'll take your advice and make 2 separate PRs of this.
Also I won't include compiled assets - them being included in the repo instead of .gitignored misled me into thinking they are required for each PR 🤦‍♂️

@SharakPL SharakPL closed this Feb 28, 2024
@SharakPL SharakPL deleted the fix-modals branch March 1, 2024 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants