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

FO / Blockwishlist - bad display when wishlist name is too long in ta… #213

Merged
merged 6 commits into from Mar 31, 2023
Merged

FO / Blockwishlist - bad display when wishlist name is too long in ta… #213

merged 6 commits into from Mar 31, 2023

Conversation

akrambak
Copy link
Contributor

@akrambak akrambak commented Mar 27, 2023

FO / Blockwishlist - bad display when wishlist name is too long in tablet/mobile screens #29213

Questions Answers
Description? FO / Blockwishlist - bad display when wishlist name is too long in tablet/mobile screens #29213
Type? bug fix
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #29213
Sponsor company Bakhouche Akram
How to test? Front.

Copy link
Member

@0x346e3730 0x346e3730 left a comment

Choose a reason for hiding this comment

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

Hello @akrambak, thank you for your contribution. Your PR only changes a builded version of the javascript, however you should need to change the source file, now the builded one.

@akrambak
Copy link
Contributor Author

Hello @akrambak, thank you for your contribution. Your PR only changes a builded version of the javascript, however you should need to change the source file, now the builded one.

Hello, can you help me to find the path of the source file to change, a litle help, i will find it myself.

@mflasquin
Copy link
Contributor

Hello @akrambak, thank you for your contribution. Your PR only changes a builded version of the javascript, however you should need to change the source file, now the builded one.

Hello, can you help me to find the path of the source file to change, a litle help, i will find it myself.

Hi @akrambak, you can see the wishlistcontainer is build with webpack, it contains files describes in .webpack/common.js :

wishlistcontainer: [ './_dev/front/js/container/WishlistContainer', './_dev/front/js/components/Create', './_dev/front/js/components/Delete', './_dev/front/js/components/Toast', './_dev/front/js/components/Share', './_dev/front/js/components/Rename', ],

So you have to update one of these file to fix the issue 😄
It it clear ?

@akrambak
Copy link
Contributor Author

@mflasquin It's clear thks
I pushed a new commit 9b6a769
Is it okay ?

@0x346e3730

@kpodemski
Copy link
Contributor

@akrambak almost! seems that you compiled assets in the dev mode, only by running the watch, is that true? you should run a build, so that assets get minified during compilation :)

@akrambak
Copy link
Contributor Author

@akrambak almost! seems that you compiled assets in the dev mode, only by running the watch, is that true? you should run a build, so that assets get minified during compilation :)

For the first commit i edited the bundle compiled directly.
In the second commit i changed the source file.
I didn't compiled.

@akrambak
Copy link
Contributor Author

@kpodemski i miss a ";"
Now it compile correctly, new commit cc2d57f

My Bad

Copy link
Contributor

@florine2623 florine2623 left a comment

Choose a reason for hiding this comment

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

Hello @akrambak ,

Thanks for your PR.

I have installed your PR, I have an exception :
Screenshot 2023-03-29 at 15 29 41

I removed the rebase with

git rebase --abort
git merge --abort
git reset --hard

When I try to configure the module I have another exception :
Screenshot 2023-03-29 at 15 28 17

Could you check ?
Thanks!

@akrambak
Copy link
Contributor Author

@florine2623 i try a new commit
Build successful
I didn't touch to the blockreassurance, so your error seems to be not linked to my commit.

@akrambak
Copy link
Contributor Author

@kpodemski can you rerun the tests please ?

@kpodemski
Copy link
Contributor

thanks @akrambak I re-run the tests, I think everything's ok with your PR

let's wait for QA to re-test it :)

Copy link
Contributor

@florine2623 florine2623 left a comment

Choose a reason for hiding this comment

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

Thanks @akrambak ! No more exception displayed !

I have retested, but I still encounter the original issue.

Your PR is well installed. I have rebuild the assets. Cleared cache from PS and from my browser. Am I missing something ?

cd _dev
rm -rf node_modules
npm i --verbose
npm run build
Screen.Recording.2023-03-31.at.13.57.24.mov

@akrambak
Copy link
Contributor Author

@kpodemski Plz relanche the again. 3ab1489
@florine2623 the ";" was the last problem.
This time it will be Ok

image

@akrambak
Copy link
Contributor Author

@kpodemski a new commit with "LF"
41bd361

@kpodemski
Copy link
Contributor

thanks @akrambak

@florine2623 florine2623 self-assigned this Mar 31, 2023
Copy link
Contributor

@florine2623 florine2623 left a comment

Choose a reason for hiding this comment

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

Thanks @akrambak ,

It is looking good :)

Checked on multiple screens sizes (desktop, tablet and mobile), it always looks as expected !
The buttons still work as usual.

Screenshot 2023-03-31 at 15 34 22

Screenshot 2023-03-31 at 15 34 30

It is QA ✅ !

@kpodemski kpodemski added this to the 3.0.1 milestone Mar 31, 2023
@kpodemski kpodemski merged commit 01152ad into PrestaShop:dev Mar 31, 2023
6 checks passed
@kpodemski
Copy link
Contributor

thank you @akrambak ! well done 👏🏻

@akrambak akrambak deleted the fo_bug_bad_wishlist_name_too_long branch March 31, 2023 13:52
@matks matks added this to the 3.0.1 milestone Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants