Update Cart & Checkout heading styles #2597
Conversation
Aljullu
added
the
focus: components
|
Size Change: -52.3 kB (3%) Total Size: 1.73 MB
ℹ️ View Unchanged
|
|
Nice seeing some weight improvements, also closes #2142 |
There was a problem hiding this comment.
In general the changes seem okay Albert. I do have a comment regarding the naming of the new component that I wonder about.
Also I noticed the following when testing (I'm testing on Storefront 2.5.7RC1 and this branch):
In the above screenshot, the focus border on the coupon panel is overlapped by the contents of the panel when opened. Not sure if this is intended or not, but looks weird.
In the following two screenshots (one desktop, and one mobile) I'm intending to illustrate the size of the titles (eg "Shopping Cart" and "2 items"). The weight of these titles doesn't seem to be that distinctive from the other content in the cart/checkout. Is that intentional? Also, even though they are h2's they don't seem to be inheriting from h2 styles in the theme (again, intentional?)
Since much of this pull is related to design, I wonder if we should also get @LevinMedia 's feedback on it as well?
| */ | ||
| import './style.scss'; | ||
|
|
||
| const DisclosureWidget = ( { |
There was a problem hiding this comment.
I'm not sure on the name of this component. Disclosure connotes something privacy or terms and conditions related, and Widget suggests a generic action tool.
Maybe InformationPanel or DescriptionPanel? Or even just Panel (while realizing the potential confusion with the WordPress component, I think if we're dropping usage of that anyways, this shouldn't be that much of an issue).
There was a problem hiding this comment.
Sounds good. Renamed to Panel in 92be5de.
|
Thanks for the review @nerrad!
Good catch! I made the button clickable area a bit smaller in 48ace85. I think the target area is still large enough so a11y is not affected and visually it looks better:
I think that's what we want per-design (tagging @LevinMedia in case he has some more feedback). The goal is to inherit all heading styles and only override the font-size. That means that in Storefront the titles have a light font weight. If necessary, we could add some styles in Storefront targeting specially these titles so they have a bolder font weight. In parallel, we could add some docs for 3rd-party themes that want to do the same.
They should inherit everything except for the size. Is there any other style they are not inheriting? |
Ahh, I missed that, in that case it appears to be working as expected. Let's wait for feedback from @LevinMedia on the other questions (in case there are any changes) and then I'll do the final review/run through. |
|
Thanks for testing this too @senadir. However, I'm unable to reproduce. This is how it looks for me with 2020:
|
|
It works fine in small width but is broken on larger width |
|
I'm still unable to reproduce. What do you mean by larger width? I tried the Full Width Template and it looks fine: |
|
Thanks for checking more themes Nadir! I also notice some issues with Twenty Twenty (Chrome on Mac): The following appears on both the default template width and the full-width template:
The following appears in the editor:
Also, even with full-width template selected for twenty-twenty, the checkout is constrained to the mobile view in the editor (might have to do with the container width in the editor? Or potentially a bug with twenty-twenty). I tested with the latest twenty-twenty theme. |
|
The latest issues make me wonder if we should settle on a common set of themes we test styles against as a part of this work to ensure we're not introducing potentially worse behaviour with changes? I fully realize getting things working well across all themes is going to be hard (and in some cases, simply impossible) but if we have a common sampling of various theme variations that can be used for the testing, it could help improve for most themes. Also, we should consider looking into automating some visual regression testing against whatever theme sampling we decide on (might be good for a cooldown). Since so much of what we are stepping is affected by themes and frontend styles, I think it will be helpful for us to automate this as a part of our transitory work between old styles and eventual Global Styles/block themes. |
|
Okay so this is an edge case as far as I see, if I insert the block alone directly, it won’t show, if I have the block inside a column (my initial setup), it will look like this
tested on master and it also existed there, so not blocking |
on an individual basis, our components works fine for the most time, the issues we run into is other themes styles leaking into ours, we can look into automating this in storefront and see how things work out. |
Oh, right. Express shipping methods don't appear by default in my local install and I missed it. 🤦
I think this was also happening in master, but this PR is a good opportunity to fix it: 3995214.
Ah, good catch! Should be fixed in the same commit. Twenty Twenty seems to add selectors with a very high specificity and that was not playing well with our blocks.
My guess is that this is an issue with TwentyTwenty. Ideally, the editor available space should change depending on the select template (this is how it works with Storefront). I created an issue in Trac: https://core.trac.wordpress.org/ticket/50311.
💯
I think @nerrad's point (or at least what I understood 😄 ) was to have an automated tool to report changes in a set of themes. Most of us are usually in Storefront so it's easy to spot regressions there, but a regression in another theme might pass inadvertently (like the |
That 👍 |
There was a problem hiding this comment.
Thanks Albert for the changes, looks much better in Twenty Twenty now. I'm going to pre-approve this but I still think we should get feedback from @LevinMedia before it's merged. Since I'm not sure he's seen this you might want to ping him directly in slack to make sure he's aware.
@nerrad I've noticed this behavior on a number of themes... and even on themes that do allow for the full width of the cart and checkout on desktop, sometimes they get positioned off to the side in a strange way. It's been discussed previously in slack with @senadir and @haszari - but I feel like this is a great case for introducing some kind of control that breaks the blocks out of the theme template, and forces them into a width and pattern of responsive behavior that we have complete control over. |
@nerrad as @Aljullu yes this is intentional... it's highly subjective to what the theme prescribes as a heading style. Here are a few examples: Storefront: Galleria Interesting to note the right column headings didn't pick up the styles correctly 🧐 Envo Astra You can see, in some cases it's great, and others it's not that rad. I think in general though it should be up to a theme to provide heading styles that contrast reasonably with body copy. All that said - @Aljullu can you bump up the font size from |
I think even that bump will help significantly as it will help distinguish it a bit more from the rest of the content imo. |
Aljullu
force-pushed
the
update/checkout-headings
branch
from
3995214
to
23e6beb
Compare
Aljullu
force-pushed
the
update/checkout-headings
branch
from
23e6beb
to
397ee6e
Compare
Ah, good catch. We weren't resetting the italics added by the
Done in 11c856b. It includes some small tweaks that were needed to make sure everything still aligned with the new heading size. Lastly, I also pushed a commit adding padding to the address form and Shipping options in the Cart block, so they align with the rest of the column:
|
* Move order summary styles to component style.scss * Fix wrong class name in Order summary * Move express payment methods a little bit higher * Fix shipping options panel misaligned in the Cart sidebar * Add right padding to panel button so text doesn't overlap the arrow * Fix wrong class name in Order summary (II)
Fixes #2562.
Closes #2141.
Closes #2142.
Part of #2459.
Part of #2605.
This PR:
<Panel>component from Gutenberg with a custom one.Panelhad some issues:<h2>, that was not semantically correct because in some cases it needed to be an<h3>(shipping packages, for example).<Card>component following to the designs in: pbIy4N-dY-p2.Accessibility
Screenshots
Cart:
Checkout:
How to test the changes in this Pull Request:
Panel→DisclosureWidgetchange. Click on theOrder Summarytitle, theCoupon?, etc.Cards in the sidebar.Changelog