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 visual issues new product page #33567

Merged

Conversation

boherm
Copy link
Member

@boherm boherm commented Aug 8, 2023

Questions Answers
Branch? 8.1.x
Description? Fix some issues in new product page.
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
How to test? See #32214
Fixed ticket? Fixes #32214
Related PRs
Sponsor company PrestaShop SA

@boherm boherm requested a review from a team as a code owner August 8, 2023 08:05
@prestonBot prestonBot added 8.1.x Branch Bug fix Type: Bug fix labels Aug 8, 2023
matks
matks previously approved these changes Aug 9, 2023
Copy link
Contributor

@Hlavtox Hlavtox left a comment

Choose a reason for hiding this comment

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

Quick check/test

1 - If you have a multi-lang site, language iso-codes should be uppercase (this is a feature request ;-)) FIXED
2 - Element below indicates whether the field is required or not, but there's no label explaining that. FIXED, but I would switch the required and delete button, delete should be at the end.
3 - What do you want me to type there? Missing placeholder. - FIXED
4 - Link below is misaligned. It is not in the center, it is not to the left, etc. FIXED
5 - There are a few issues there. The help text doesn't look like a help text, warning/info text doesn't look like one. IMPROVED
6 - Worth considering making it more readable. (feature request) - FIXED, collapse added
7 - When you click this button, it gets a weird border covering part of the text. 🔴 PARTIALLY FIXED - @boherm add some margin under the helptext or something, or margin top on the button?
8 - I don't understand this part. What is this currency for? I have PLN but still the EUR symbol on the left. (edit, ok, it doesn't change dynamically). 🔴 NOT FIXED?

@boherm
Copy link
Member Author

boherm commented Aug 9, 2023

Hi @Hlavtox, thanks for your review ;)

7 - When you click this button, it gets a weird border covering part of the text. 🔴 PARTIALLY FIXED - @boherm add some margin under the helptext or something, or margin top on the button?

Yeah, why not to add a margin.
I just add the "disable" button behavior thing when we don't have a change on the friendly url.

8 - I don't understand this part. What is this currency for? I have PLN but still the EUR symbol on the left. (edit, ok, it doesn't change dynamically). 🔴 NOT FIXED?

Weird 🤔 Have you correctly rebuild the new theme?

@Hlavtox
Copy link
Contributor

Hlavtox commented Aug 9, 2023

@MatShir @TristanLDD Guys one more improvement regarding the specific prices.

  • Disable one checkbox, if you enable the other one?
  • What about adding A) and B) or something before the two discount options?

(automatic translation in chrome)
Snímek obrazovky 2023-08-09 181954

@MatShir
Copy link
Contributor

MatShir commented Aug 10, 2023

If I remember well both can be applied so, that's why it is not like a switch behavior.

@boherm
Copy link
Member Author

boherm commented Aug 10, 2023

Yo @Hlavtox, I've fix the 7th and 8th items.
For the 8th, I don't reproduce on my side but maybe it's a browser compatibility issue.
Anyway, with the fix I've done, this could be working on your side now.

If you have some time, can you recheck that? 🙏

@boherm boherm force-pushed the #32214-fix-visual-issues-new-product-page branch from 44934f1 to 5b523d4 Compare August 10, 2023 15:46
@ps-jarvis ps-jarvis added the Waiting for QA Status: action required, waiting for test feedback label Aug 11, 2023
@Hlavtox
Copy link
Contributor

Hlavtox commented Aug 11, 2023

@boherm I will test it during the weekend 👍

Copy link

@aniszr aniszr left a comment

Choose a reason for hiding this comment

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

Hello @boherm

I tested your PR, almost all of your improvements are well displayed.

See attached screenrecord:

Products.Prestashop81x.mp4

The only failed improvement is :

  • Mobile -> Tab Stock -> Fix display for Add or subtract item input ❌
    See attached screenshot:
    image

Could you please check and feedback!

Thanks!

@aniszr aniszr added Waiting for author Status: action required, waiting for author feedback and removed Waiting for QA Status: action required, waiting for test feedback labels Aug 11, 2023
@Hlavtox
Copy link
Contributor

Hlavtox commented Aug 11, 2023

@aniszr Anis, does this work? You must show it. :)

IMG_7307

@aniszr
Copy link

aniszr commented Aug 14, 2023

Hello @Hlavtox

Thanks for your feedback.

I retested the currancy changing behavior and found that Cost price (tax excl.) don't change automatically the currency symbol when we assign a new Currency! Unless we click save and publish!!

See attached screenrecord:

Localization.Prestashop81x.mp4

Thanks!

@boherm
Copy link
Member Author

boherm commented Aug 22, 2023

Hello @aniszr,
Have you really rebuild assets (and precisely admin-new-theme) before testing? 🤔
It's reaaaaaally weird...

@prestashop-issue-bot prestashop-issue-bot bot removed the Waiting for author Status: action required, waiting for author feedback label Aug 22, 2023
@MatShir MatShir added the Waiting for QA Status: action required, waiting for test feedback label Aug 23, 2023
Copy link

@aniszr aniszr left a comment

Choose a reason for hiding this comment

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

Hello @boherm

I retested your PR and make sure that cache is cleared and admin-new-theme is well generated ✔️

The previousely mentioned problems are solved :

  • Mobile -> Tab Stock -> Fix display for Add or subtract item input ✔️
  • The Cost price(tax excl.) currency symbol is responsively changed when changing Currency field ✔️

See attached screenrecord:

Inbox.90.-.anis-zouari.ext@prestashop.com.-.Prestashop.SA.Mail.mp4

Auto Test : https://github.com/aniszr/ga.tests.ui.pr/actions/runs/5832815208 ✔️

QA ✔️

Thank you!

@aniszr aniszr added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Aug 23, 2023
@prestonBot
Copy link
Collaborator

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

@MatShir
Copy link
Contributor

MatShir commented Aug 23, 2023

🎉 thanks everyone

@MatShir MatShir added this to the 8.1.2 milestone Aug 23, 2023
@boherm boherm merged commit 61f7359 into PrestaShop:8.1.x Aug 23, 2023
38 checks passed
@boherm boherm deleted the #32214-fix-visual-issues-new-product-page branch August 23, 2023 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.1.x Branch Bug fix Type: Bug fix QA ✔️ Status: check done, code approved
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

8 participants