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

Responsive visual issues new product page #35775

Open
wants to merge 21 commits into
base: 8.1.x
Choose a base branch
from

Conversation

mattgoud
Copy link
Contributor

@mattgoud mattgoud commented Mar 28, 2024

Questions Answers
Branch? 8.1.x
Description? see #32218
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
How to test? go to product page and test each tab (fork : https://github.com/mattgoud/PrestaShop/tree/fix/32218-responsive-visual-issues-new-product-page)
UI Tests https://github.com/florine2623/testing_pr/actions/runs/8797116451
Fixed issue or discussion? Fixes #32218 (with the exception of point number 4 : creation of a separate issue see #35765
Related PRs If theme, autoupgrade or other module change is needed to make this change work, provide a link to related PRs here.
Sponsor company Your company or customer's name goes here (if applicable).

@mattgoud mattgoud requested a review from a team as a code owner March 28, 2024 15:53
@prestonBot
Copy link
Collaborator

Hello @mattgoud!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

@prestonBot
Copy link
Collaborator

Hi, thanks for this contribution!

I found some issues with the Pull Request description:

  • Your pull request does not seem to fix any issue, consider creating one (see note below) and linking it by writing Fixes #1234.

Would you mind having a look at it? This will help us understand how interesting your contribution is, thank you very much!

About linked issues

Please consider opening an issue before submitting a Pull Request:

  • If it's a bug fix, it helps maintainers verify that the bug is effectively due to a defect in the code, and that it hasn't been fixed already.
  • It can help trigger a discussion about the best implementation path before a single line of code is written.
  • It may lead the Core Product team to mark that issue as a priority, further attracting the maintainers' attention.

(Note: this is an automated message, but answering it will reach a real human)

@prestonBot prestonBot added 8.1.x Branch Bug fix Type: Bug fix labels Mar 28, 2024
@mattgoud mattgoud force-pushed the fix/32218-responsive-visual-issues-new-product-page branch from 6d35420 to 8f41bd1 Compare March 28, 2024 16:29
Comment on lines 482 to 484
.material-icons {
margin-top: 0;
}
Copy link
Contributor

@SharakPL SharakPL Mar 28, 2024

Choose a reason for hiding this comment

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

I really doubt this requires a selector made of 6 components, including 2 IDs 😱
obraz

Did you try adding mt-0 class to that element?

I'm guessing it's not that easy since it's probably nested in some twig partial used in many places, but surely it can be shortened to 2-3 components selector to be specific enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would go for utility classes where we can, maybe not the most performant solution, but it's much safer making changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hello, ok this is a somewhat specific need (on a single go-to-catalog button). I have reduced the depth of the classes nesting if you prefer.

@jolelievre jolelievre linked an issue Apr 9, 2024 that may be closed by this pull request
2 tasks
jolelievre
jolelievre previously approved these changes Apr 9, 2024
Copy link
Contributor

@jolelievre jolelievre left a comment

Choose a reason for hiding this comment

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

Thanks @mattgoud

boherm
boherm previously approved these changes Apr 9, 2024
@WahbiPS WahbiPS added the Waiting for QA Status: action required, waiting for test feedback label Apr 19, 2024
@florine2623 florine2623 self-assigned this Apr 19, 2024
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 @mattgoud ,

Tested on multiples devices as well as on desktop.

  1. Save and publish display problem. ✅
Screenshot 2024-04-23 at 11 00 19
  1. Three dots icon is not aligned correctly. ✅
Screenshot 2024-04-23 at 11 01 59
  1. We could use a little more spacing there. ✅
Screenshot 2024-04-23 at 11 04 19
  1. Features are displayed in a very confusing way. ✅
Screenshot 2024-04-23 at 11 06 38

But on Desktop, the trash icon is not aligned. It should be on the same line as the input ❌ :
Screenshot 2024-04-23 at 11 33 12

  1. Customizable product. Problems with spacing and alignment. ✅
Screenshot 2024-04-23 at 11 08 12
  1. It is impossible to edit the stock on a mobile device. ✅
Screenshot 2024-04-23 at 11 10 17
  1. Alignment problem. ✅
Screenshot 2024-04-23 at 11 12 40
  1. Incorrectly displayed prices. ✅
Screenshot 2024-04-23 at 11 13 40
  1. Cost price displayed incorrectly (I'm aware that it's a tricky element to display, maybe a responsive table would be better). ❌ The Currency dropdown is displayed too high.
Screen.Recording.2024-04-23.at.11.28.54.mov

Could you check points 3 and 9 please ?
Thanks :D

@florine2623 florine2623 added the Waiting for author Status: action required, waiting for author feedback label Apr 23, 2024
@florine2623 florine2623 removed their assignment Apr 23, 2024
@mattgoud mattgoud changed the title Fix/32218 responsive visual issues new product page Responsive visual issues new product page Apr 26, 2024
@MatShir
Copy link
Contributor

MatShir commented Apr 26, 2024

Number 3 isn't the native behavior ? @mattgoud
Maybe not worth changing the native implementation if it is a native behavior 🤔

@Hlavtox
Copy link
Contributor

Hlavtox commented Apr 26, 2024

@MatShir He only fixed the responsivity, no change in behavior, all good.

@mattgoud mattgoud dismissed stale reviews from boherm and jolelievre via 12ae5e2 April 26, 2024 10:52
@mattgoud mattgoud force-pushed the fix/32218-responsive-visual-issues-new-product-page branch 2 times, most recently from 12ae5e2 to 5c10baf Compare April 26, 2024 11:54
@MatShir
Copy link
Contributor

MatShir commented Apr 26, 2024

I was talking of the number 9 🙄

@MatShir MatShir removed Waiting for author Status: action required, waiting for author feedback Waiting for QA Status: action required, waiting for test feedback labels Apr 26, 2024
@mattgoud
Copy link
Contributor Author

I was talking of the number 9 🙄

ok thank you for your feedbacks @MatShir @Hlavtox , yes on mobile it will be a native select. @florine2623 I will only deal with number 3

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
Projects
Status: PRs
Development

Successfully merging this pull request may close these issues.

Visual issues on the new product page (mobile)
9 participants