-
Notifications
You must be signed in to change notification settings - Fork 144
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Left a few comments
Just noting that the original design spec has them as fluid/full width to match the table/components - so that's what we've gone with |
@j111q @danielbitzer I opted to go with $theme-color - I thought a few times working on this we should just keep things consistent for the time being |
@jconroy @danielbitzer Great points and thank you for the thoughts. Yes, I think going with $theme-color is the best solution too. We did want to introduce the new blue, but I'd agree it's jarring, and since the new WC Home page does the same, let's align with using $theme-color instead. Re: max-width of cards, yes, I believe the original design intended to match the width of the table, which I'd agree with. If we introduce a max-width for the cards, I'd like to have a max-width for the table and other components like the notice banner as well. |
Updated based on feedback and re-tested in browsers including edge and ie11 Also flipped to using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also flipped to using
var(--wp-admin-theme-color
) instead of$theme-color
This doesn't seem to be working on the Coupons page. I don't see any hover affect in my local testing.
I've also noted a few other issues I came across.
One other thing that's not quite in the scope of the PR: I noticed that we're making use of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Looks like CSS variables like |
So WC Admin's minimum supported WP version is 5.3 which means the min Gutenberg version is 6.5. And it looks like the icons package was added in Gutenberg 7.4
Ahh right now I understand what you mean. So I **do need** to bundle the other 3 right?
|
I'm afraid so. Sorry I should have checked compatibility before I suggested using |
@danielbitzer All good! its more my own inexperience, thanks for taking a look. I've added them in. |
In progress work for updating styles used on the marketing tab and coupons pages to reflect designs below
Changes
Screenshots
Other
Detailed test instructions:
Changelog Note:
Tweak: Style improvements for Marketing hub.