Introducing consistency in colors for deposits across pages #8382
Conversation
naman03malhotra
changed the title
naman03malhotra
changed the title
|
@rogermattic can you please review the colors as part of this change? |
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Team, I want to double-check, as:
|
|
Size Change: +35 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
Manual testThe colors seem as expected for This is not for us, but a general feedback on the WP color. This may be my eyesight or the screen too. The dark green appears not very contrastingly different from the black font. Strangely on the screenshot it looks better, but not in the actual browser.
|
client/deposits/details/style.scss
Outdated
| } | ||
|
|
||
| &.is-pending, |
There was a problem hiding this comment.
I couldn't find a canceled state showing in grey in the issue description.
There is an image shared in the issue with slightly different brighter shades ( probably studio ) . I had to recopy this image here since it had some weird private URL.
We may want to use the wp-admin shade equivalents, that are slightly darker / a bit dull compared to the screenshot in the issue. @rogermattic may kindly reconfirm.
Secondly, in transit and pending states are both yellow.
As per the Acceptable criteria in the issue, we need to use shades of blue for in transit.
So maybe
| &.is-pending, | |
| &.is-pending { | |
| background: $wp-blue-70; | |
| border-color: $wp-blue-0; | |
| } | |
| &.is-canceled { | |
| background: $wp-gray-70; | |
| border-color: $wp-gray-blue; | |
| } |
The gray color may end up looking almost black, though.
We probably need a different shade?
|
Left a couple of comments. Re-inviting Helix team too since I was unable to do a manual test for |
There was a problem hiding this comment.
What I understand about the ask from the issue #7761 is that we need to have a consistent coloring for each of the deposit state shown on the deposits list page and deposits details page.
So here's what I did to confirm that:
- Check the status color on the list page
- Make a local change here and pass status
paid,pending,in_transit,failed, orcanceled, so I don't have to create deposit in all those possible states. - I inspect the status and see the color code, eg for
paidstatus, the chip element has color#005c12.
- Make a local change here and pass status
- Check the status color on the details page
- Make a local change here and pass status
paid,pending,in_transit,failed, orcanceled, so I don't have to create deposit in all those possible states. - I inspect the status and see the background color code, eg for
paidstatus, theis-paidclass has background color#005c12.
- Make a local change here and pass status
<OrderStatus order={ { status: "paid" } } orderStatusMap={ displayStatus } />
Here are what I found that need fixing:
- When status is
in_transit, the details page shows yellow where it should be green like shown on the deposits list page. Also, I think settingwhite-space: nowrapwould be a good idea.
Here's how it's shown on the list page:
Here's how it's shown on the details page:
- When status is
canceled, the details page shows grey where it should be red like shown on the deposits list page.
Here's how it's shown on the list page:
Here's how it's shown on the details page:
Also, I think you are aware already but I like to mention that there are WordPress colours defined as constants here in the repo. I just learned about this as pointed out by @Jinksi's here p1709158123401059/1709146807.215419-slack-C02BW3Z8SHK.
|
Just came across this – does it need work or is it ready for merge? @shendy-a8c can you take a look and make a plan to follow up (yourself or assign)? |
|
Thank you for checking, @haszari. It needs work. It's on my radar but pretty low compared to other things on my plate. ETA is late this week or next week. |
|
I have made updated to this PR to address reviewer comments and match the intended design with WP colors – please see the new PR description. |
There was a problem hiding this comment.
Code changes look good and make good reuse of WP colors. The rendered colors also match the requirements of the original issue.
Thanks for providing the testing patch, @Jinksi ! It was very helpful! Thank you, also for taking over this long-standing issue 🙏🏼
Manual tests run well too, and this PR is ready to Ship 🚢
|
@nagpai @shendy-a8c @Jinksi @haszari thanks for working on this! I like the colours and the consistency. One thing that I want to mention is that once the |
|
Awesome @rogermattic 🚀 It looks like the new It looks like WooPayments is way back on Unfortunately, WooPayments has not kept up-to-date with component library versions as part of regular maintenance, and so an update to the current version will require effort that has not been easy to prioritise in the past – @Automattic/helix talked about this at our last meetup pdjTHR-3SB-p2. We'll be conducting a related spike soon (#8012) which may result in some next steps to resolve the situation here. |
Fixes #7761
Commandeered by @Jinksi 🏴☠️
Changes proposed in this Pull Request
There are 3 changes in this PR:
(1)
Updated the Deposit Status Chip colors to the following:
Pending 🟡
In transit 🔵 – was 🟡
Paid 🟢
Failed 🔴
Canceled ⚪ (gray) – was 🔴
(2)
Updated the Deposit Details screen status indicator colors to be consistent with the status chip colors using standard WP admin colors.
Before / After
(3)
I've also added
white-space: nowrap;to the status indicator element to ensure that "In transit" is shown on a single line for improved readability.Testing instructions
git apply mock_deposits.patch: mock_deposits.patch and then runnpm run build:clientnpm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge