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

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

Open
filipefurtad0 opened this issue Jan 11, 2023 · 15 comments · May be fixed by #13010
Open

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

filipefurtad0 opened this issue Jan 11, 2023 · 15 comments · May be fixed by #13010
Assignees
Labels
bug-s5 We can live with it... Few users are impacted. good first issue hackathon Issues for upcoming hackathons regression Tagging any identified regressions

Comments

@filipefurtad0
Copy link
Contributor

filipefurtad0 commented Jan 11, 2023

Description

For editable orders - i.e., orders which can be cancelled or line items amount changed by the customer, while the OC is open - the table in the order confirmation page shows an inconsistent format.

Expected Behavior

Table format should be consistent.

Actual Behaviour

Table formatting is inconsistent (blank space is visible).

Steps to Reproduce

  1. As an admin: Set up a hub to accept order edits - Under Shop Preferences toggle "Customers can change / cancel orders while order cycle is open"
  2. As a customer: Place an order on that Hub
  3. As a customer: Edit that order - visit /account#/orders and click Edit for that order -> Notice the blank space on the table.

(This affects only order confirmation page while the OC is open:
4. As an admin: Close the order cycle
5. As a customer: Edit that order -> notice the formatting is correct)

OR:

Skip all steps and visit:
https://staging.openfoodnetwork.org.uk/orders/R632653401?order_token=a629d72866356506

Animated Gif/Screenshot

image

Workaround

Does not affect functionality.

Severity

bug-s5: we can live with it, only a few users impacted

Your Environment

  • Version used: v4.2.29
  • Browser name and version: Firefox 108.02 (does not affect Chrome)
  • Operating System and version (desktop or mobile): Ubuntu 22.04

Possible Fix

@filipefurtad0 filipefurtad0 added bug-s5 We can live with it... Few users are impacted. regression Tagging any identified regressions labels Jan 11, 2023
@drummer83
Copy link
Contributor

Maybe introduced when we added 'Back to Website' button (not visible on your screenshot)?

@filipefurtad0
Copy link
Contributor Author

filipefurtad0 commented Jan 11, 2023

No, I had checked that PR at the time, it was fine - it's broken now:
image

This could just be a browser issue (updated the issue). Not sure.

@jibees
Copy link
Contributor

jibees commented Jan 12, 2023

I'm not sure to know how to reproduce this one...
by As a customer: Edit that order, what do you mean?
This is what I get:

Capture d’écran 2023-01-12 à 09 26 35

@filipefurtad0
Copy link
Contributor Author

Hey @jibees ,

Sorry, I guess I should have been more explicit 🙈 I've edited above.
If the enterprise is set accordingly (step 1) and the order cycle is still open then, the customers can edit orders. Do to so, log in as a customer and visit /account#/orders and click Edit for that order:

image

This will lead the user to the order confirmation screen.

@jibees
Copy link
Contributor

jibees commented Jan 12, 2023

Found it.
This is due to #10103
and that particular line #14:

#cart-detail {
width: 100%;
display: block;

the <table /> is therefore display: block; instead of its inherit display which is display: table;

@RachL
Copy link
Contributor

RachL commented Jan 13, 2023

FYI this is reproductible on order confirmation page as well. I've just tested on production:

image

@filipefurtad0
Copy link
Contributor Author

FYI this is reproductible on order confirmation page as well. I've just tested on production:

Yes, I can see it too in order confirmation pages (either editing or after checkout), but only for editable orders. Is that the case, on that pic @RachL ?

@RachL
Copy link
Contributor

RachL commented Jan 13, 2023

Yep!

@github-project-automation github-project-automation bot moved this to All the things in OFN Delivery board Feb 16, 2024
@github-project-automation github-project-automation bot moved this to To triage (By the maintainers) in Welcome New Developers! May 14, 2024
@sigmundpetersen sigmundpetersen changed the title [Customer] Inconsistent table format on (editable) order confirmation page [Customer][Order->Edit] Inconsistent table format on (editable) order confirmation page Jun 5, 2024
@dacook dacook moved this from To triage (By the maintainers) to Frontend Easy in Welcome New Developers! Jul 18, 2024
@ashwini-seshadri
Copy link
Contributor

Hello, is this issue still open? If so, I would love to work on this! :) Thank you!

@sigmundpetersen
Copy link
Contributor

Sure, thank you @ashwini-seshadri 👍

@sigmundpetersen sigmundpetersen moved this from Frontend Easy to In progress in Welcome New Developers! Aug 15, 2024
@sigmundpetersen sigmundpetersen moved this from All the things 💤 to In Progress ⚙ in OFN Delivery board Aug 15, 2024
@drummer83
Copy link
Contributor

Thank you @ashwini-seshadri!
I have just checked. The problem is still there, but it's smaller now - at least on my device and browser (Firefox).

Here is a screenshot:
image

@RachL RachL added the hackathon Issues for upcoming hackathons label Sep 24, 2024
@RachL RachL moved this from In progress to Frontend Easy in Welcome New Developers! Oct 1, 2024
@RachL RachL moved this from In Progress ⚙ to All the things 💤 in OFN Delivery board Oct 1, 2024
@ndossougbete
Copy link

I can still repro the gap on Firefox 130.0.1, but as mentioned in the original message, it doesn't happen on Chrome.

I also found as potential fix the same thing that was brought up in #10261 (comment). The display: block lines added in #10103 for both split-checkout.scss #line-items and cart-page.scss #cart-detail don't seem needed anymore to make the screen wrap on mobile, so maybe it's fine to remove it after all.

2024-11-01_17-16

If this issue is still up for grab, can I look into preparing a PR for these changes?

@sigmundpetersen
Copy link
Contributor

Sure go ahead @ndossougbete 👍
I'll assign you. Thanks!

@sigmundpetersen sigmundpetersen moved this from All the things 💤 to In Progress ⚙ in OFN Delivery board Nov 1, 2024
@sigmundpetersen sigmundpetersen moved this from Frontend Easy to In progress in Welcome New Developers! Nov 1, 2024
@ndossougbete
Copy link

I took some time to look into it today, and I need to correct the statement in my comment above. I think it will be more complicated. The fix added in PR #10103 to address issue #9976 does not seem to be working anymore.

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

One one hand I don't quite understand why the change to make the table have display: block does not work, but on the other hand I think it's not going to be the right solution. It is mentioned on MDN, but with a caveat:

you'll notice that table's display property has been set to block. While this allows scrolling, the table loses some of its integrity, and table cells try to become as small as possible.

Chrome seems to handle the "cells try to become as small as possible" part better than Firefox, which is why a gap can show up. I found an article mentioning that it might cause some accessiblity issues. And overall, the whole body still scrolls on mobile Chrome & Firefox when a long string without spaces is included in the table. On Firefox is extra bad, compare outcome of executing the snippet here: https://stackoverflow.com/a/48736763. Other difference, in Chrome the "Back to store" button seems to have the wrong size, while in Firefox it matches the size of the buttons below.
Screenshot from 2024-11-11 21-07-47

I'll have another look when I have some time. Hopefully I missed some simple fix, but maybe it will take a more comprehensive rewrite of the markup to fix this. Maybe CSS-calc'ing the columns get elements with the right sizes?

All that to say that I'll hold off of submitting a PR just removing the display: block properties, it seems irrelevant to do considering the other issues, and even makes things worse in certain cases 😅

And please let me know if I should fork the general table/scrolling issues of these screens to a separate issue.

@ndossougbete ndossougbete linked a pull request Dec 1, 2024 that will close this issue
1 task
@ndossougbete
Copy link

Hi! I gave up on trying to get the table to work on mobile, and created PR #13010 to fix the bug initially mentioned here.

I think the table will never work properly on mobile and it would be best to try an alternate layout to display this info without needing to scroll. An interpretation of this (that still needs a lot of work to be anything close to shippable) could be something like:
Screenshot from 2024-12-01 16-48-17.
It removes some columns and attempts to put the info back in the first cell instead. But that's becoming off-topic, I think the mobile tables should be forked to a separate issue (or reopen #9976 maybe?) if this is to be done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-s5 We can live with it... Few users are impacted. good first issue hackathon Issues for upcoming hackathons regression Tagging any identified regressions
Projects
Status: In Progress ⚙
Status: In progress
Development

Successfully merging a pull request may close this issue.

7 participants