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 gaps in cart & checkout tables on Firefox #13010

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ndossougbete
Copy link

What? Why?

Tables from the cart and checkout pages would show some gaps where the table content doesn't extend to take all the available space. Not forcing the display CSS property of table elements to block appears to fix the issue without adverse side effects.

before after
order_edit_table_before order_edit_table_after

The display: block properties were originally added in PR #10103 to address issue #9976 on mobile, but it does not seem to be working anymore, with or without this patch. For example, in the screenshot below, the white gap at the right of the footer hints at how much extra width is added on the page:
mobile_viewport_content_too_wide

PR #10103 was aiming to make the table itself scroll horizontally while keeping the overall body width matching the viewport. It doesn't work today, and upon testing, seems unfeasible on Firefox (see the issue mentioned on https://stackoverflow.com/a/48736763, compare behaviour on Chrome & Firefox). If it was working, it would be sometimes confusing as there would be no or very little indication that the table has the ability to scroll. I would suggest to entirely change the table's layout on Mobile instead. For example, put price, quantity and the remove button below the item title and description, so that everything can fit without needing to scroll.

What should we test?

Repro steps:

  1. A mobile device or some way to simulate it will be needed. I used the mobile/responsive mode from dev tools. E.g. on Firefox , the Galaxy Note 20 at 412px width.
  2. As an admin: In one of the hubs, edit some of the items to make sure a few have some reasonably long name without spaces, so that it is not auto-wrapped. Example: on my simulated phone above, I observed "Mushrooms" to be long enough to cause the issue.
  3. As a customer, on a mobile device: Edit an already placed order:
    • Place an order on that hub
    • Visit /account#/orders and click "Edit" for that order -> Notice that the whole page scrolls horizontally

There is also an affected table on the cart page, but I couldn't repro the issue there.

Release notes

Changelog Category (reviewers may add a label for the release notes):

  • Technical changes only

The title of the pull request will be included in the release notes.

Tables from the cart and checkout pages would show some gaps where the
table content doesn't extend to take all the available space. Not
forcing the `display` CSS property of table elements to `block` appears
to fix without adverse side effects.
Copy link
Member

@dacook dacook left a comment

Choose a reason for hiding this comment

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

Great, thankyou for your detailed explanation, and pointing out that #9976 is not working.
We'll test this out and consider re-opening the issue.

@ndossougbete
Copy link
Author

Hello @dacook this is my first PR, can you please clarify if there is any action item for me before merging this? Or are we just waiting for more tests/verifications? Thanks!

@dacook
Copy link
Member

dacook commented Dec 17, 2024

Hi @ndossougbete, sorry for the delay, but yes we need an expert to manually test and verify this change before merging. Our testing team has limited capacity and there is a large backlog at the moment, so this will take longer than usual.

Thanks again for your contribution, it will get there eventually!

@drummer83 drummer83 self-assigned this Jan 8, 2025
@drummer83 drummer83 added the pr-staged-au staging.openfoodnetwork.org.au label Jan 8, 2025
@drummer83
Copy link
Contributor

drummer83 commented Jan 9, 2025

Hi @ndossougbete,
Thank you so much for working on this issue and for your deep investigation. 🙏

I have tested before and after staging your PR on our testing server. There are four pages I found having a similar version of the table available, so I decided to compare their behaviour.

Firefox

Page Table ID Before After
Gap Scrolling Gap Scrolling
Edit cart cart-detail No ✔️ Table (see image 1 below) ✔️ No ✔️ Page body (content readable, but broken page layout, see image 2 below) ✔️ ❌
Order summary line-items No ✔️ Table ✔️ No ✔️ Page body (content readable, but broken page layout) ✔️ ❌
Order confirmation line-items No ✔️ Page body (content readable, but broken page layout) ✔️ ❌ No ✔️ Page body (content readable, but broken page layout) ✔️ ❌
Editable order confirmation cart-detail Yes ❌ Page body (content readable, but broken page layout) ✔️ ❌ No ✔️ 🥳 Page body (content readable, but broken page layout) ✔️ ❌

Image 1

grafik

Image 2

grafik

Conclusion

This is a tricky one.
Your PR is solving the issue with the "gap" in the table on Firefox. Excellent! 🥳 💪
However, it comes at a cost. We now have the scrolling of the full page body in case of long line item names. This is breaking the page layout. Before, we would simply have a scrollbar on the line item table, which I find acceptable.
But, we have this exact behaviour with broken page layout in other places in the app anyway: order confirmation and editable order confirmation. Something must be better designed on this overall page compared to order summary and edit cart. Maybe finding this difference is the solution?

Feedback

@openfoodfoundation/train-drivers-product-owners What do you think? Is the benefit of the fixed gap greater than the cost of the broken page layout? I personally don't think so. Let us know and we will take it from there.

Testing note

Before merging this should be tested on Chrome and Safari as well.

Thanks again @ndossougbete! Let's see how we move on!

@drummer83 drummer83 added feedback-needed and removed pr-staged-au staging.openfoodnetwork.org.au labels Jan 9, 2025
@drummer83 drummer83 removed their assignment Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Test Ready 🧪
Development

Successfully merging this pull request may close these issues.

[Customer][Order->Edit] Inconsistent table format on (editable) order confirmation page
3 participants