backdrop-contrib / ubercart

A flexible but easy-to-use e-commerce system for Backdrop.
https://backdropcms.org/project/ubercart
GNU General Public License v2.0
4 stars 10 forks source link

Review order page styles don't match rest of theme #508

Closed herbdool closed 1 week ago

herbdool commented 3 weeks ago

The styling on the Review Order page doesn't quite match the rest of the theme. It's quite narrow; the font is unnecessarily small and so on.

Before:

2024-10-29 15 27 49 bd3 lndo site ecc72be91514

After:

2024-10-29 15 44 54 bd3 lndo site 30a17375a004

herbdool commented 3 weeks ago

@bugfolder what do you think of this PR?

argiepiano commented 3 weeks ago

I think this look was an outdated attempt (probably in screens with lower resolution in the late 2000s) to make it look like a cashiers receipt. It's annoying and outdated, and it looks bad on modern screens. I'm all for changing this.

bugfolder commented 3 weeks ago

I definitely like the new look.

When I tried it on an existing site, I didn't get a full-width listing (even after flushing all caches and flushing the browser CSS/JS cache).

Before:

before

After:

after

Adding width: 100%; to the table CSS gets it out to the full width, but the huge horizontal gap and tight vertical spacing actually degrades readability, IMHO.

wider

Personally, I'm fine with tweaking the CSS on the site to get that table readable (since it has a unique class of order-review-table, it's easily targetable). While I do like the improvement shown above, I'm a little concerned about substantially changing the CSS on our users who may have made their own CSS overrides that don't play nicely with the updated CSS, and being surprised that the review page changes in potentially not a good way. Thoughts?

herbdool commented 3 weeks ago

What theme are you using? I guess some smart defaults are still needed in some cases.

bugfolder commented 3 weeks ago

Base theme is Bootstrap Lite. Then there's a sub-theme, but the sub-theme doesn't do much to the order-review-table.

We probably ought to check this against a bunch of common themes.

herbdool commented 3 weeks ago

Ah, ok. I noticed that the Review Order panes were being hardcoded into a table, rather than using a renderable array. So Bootstrap Lite couldn't inject its classes. So I've rebuilt that. It's a bit more radical of a change, but I think it's worth it since it's now easier to make sure it looks nice across different themes.

Bootstrap Lite:

2024-10-31 12 01 10 bd3 lndo site 168a9013c4ae

Basis:

2024-10-31 12 00 52 bd3 lndo site 21013752e615

I also moved away from making the whole thing a giant table so now it's fieldsets with a table in each one.

I'm a little concerned about substantially changing the CSS on our users who may have made their own CSS overrides that don't play nicely with the updated CSS, and being surprised that the review page changes in potentially not a good way.

One approach would be to change the classes so that any custom css overrides won't mess with the new look.

Another would be to try to make the fieldsets look closer to the table of tables. Not sure how important that is. And won't really address any custom css overrides.

bugfolder commented 3 weeks ago

Much better:

revised

I particularly like that the "Review order" page stylistically matches the "Checkout" page, which helps the sense of continuity in the user.

I support this change.

herbdool commented 3 weeks ago

@bugfolder sounds good.

laryn commented 3 weeks ago

This is a good improvement.