Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

SSC: Fix notification colors#62860

Merged
vdavid merged 6 commits into
mainfrom
dv/ssc-fix-notification-colors
May 24, 2024
Merged

SSC: Fix notification colors#62860
vdavid merged 6 commits into
mainfrom
dv/ssc-fix-notification-colors

Conversation

@vdavid

@vdavid vdavid commented May 22, 2024

Copy link
Copy Markdown
Contributor

Our notifications had no dark theme style, and they weren't design-approved.
This PR makes them match the figma.

Caveats:

  • I had no design for the error notification style, so I came up with my best reds that matched the other colors. I also still have no icon for that style.
  • In the figma, there were illustrations on the left side of the notifications, but I assumed those were only for specific messages, so I left the checkmark and the crappy X I made.

Light theme looks:

CleanShot 2024-05-22 at 17 36 01@2x

Dark theme looks:

CleanShot 2024-05-22 at 17 34 52@2x

Test plan

Tested with a temporary state to demo. See the screenshots.

@vdavid vdavid requested review from a team and rrhyne May 22, 2024 15:41
@cla-bot cla-bot Bot added the cla-signed label May 22, 2024
@vdavid vdavid force-pushed the dv/ssc-fix-notification-colors branch from 8c7ccea to 2e71693 Compare May 22, 2024 15:59

@chrsmith chrsmith left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll defer to Rob for approval. But this all looks good to me.

QQ: For common things like purple-success-alert, is CodyManageTeamPage.module.scss the right place to define these things?

Would we be better off if we just imported these styles from another CSS file?

e.g. maybe we keep CodyManageTeamPage.module.scss but just have an @import '../alerts'; statement so we don't need to duplicate these if we need to pop up alerts on another page?

vdavid commented May 23, 2024

Copy link
Copy Markdown
Contributor Author

My thinking is this:

  • Until we actually use these on other pages, it makes more sense to keep these with the page they belong to.
  • Once we want to use them on other pages then it'd be very awkward to keep them with this page, so we'll naturally extract them.

So rather than future-proofing now, I want to organically transform the structure. It's very cheap, and the benefit is that each state is a valid one, plus, if we change our plans, we're still in a good place.

@vdavid

vdavid commented May 23, 2024

Copy link
Copy Markdown
Contributor Author

I've updated the text colors:

CleanShot 2024-05-23 at 13 40 17@2x

CleanShot 2024-05-23 at 13 40 38@2x

But then, with an eyedropper Chrome plugin, I couldn't confirm the real colors that get rendered because it seems to be broken even for the figma 🤯:

CleanShot 2024-05-23 at 13 43 00@2x

But then, in the CSS, I use the correct dark theme background colors: #d68cf3 and #51cf66. So they should be correct even if they render differently by the time the screenshot reaches the figma.

I've also replaced some literals with the matching CSS variables.

@vdavid vdavid enabled auto-merge (squash) May 23, 2024 11:50
@vdavid

vdavid commented May 23, 2024

Copy link
Copy Markdown
Contributor Author

Okay, just realized I made a mistake with not updating the checkmark icon and some text colors. Now it's like this:

CleanShot 2024-05-23 at 14 28 06@2x

CleanShot 2024-05-23 at 14 26 23@2x

The reds are a best-effort.

@rrhyne

rrhyne commented May 23, 2024

Copy link
Copy Markdown

Thanks. I did not specify a red version anywhere, I think we should remove that for now.

@vdavid

vdavid commented May 23, 2024

Copy link
Copy Markdown
Contributor Author

@rrhyne: Sure. What style shall I use for the "Invites not sent" error notification and its sisters?

@vdavid

vdavid commented May 23, 2024

Copy link
Copy Markdown
Contributor Author

...or, broader question because I might be off-track with the whole notification direction: how should I communicate it when an action (like sending the invites) fails?

@rrhyne

rrhyne commented May 23, 2024

Copy link
Copy Markdown

Roger. I put together these warning states:

https://www.figma.com/design/FMSdn1oKccJRHQPgf7053o/Cody-PLG-GA?node-id=5082-11931&t=35JUr8zVHPGQAO13-4

We shouldn't go all red there as there isn't anything critical happening.

@vdavid

vdavid commented May 23, 2024

Copy link
Copy Markdown
Contributor Author

@rrhyne the updated designs, now with the new icons:

CleanShot 2024-05-23 at 23 39 33@2x

CleanShot 2024-05-23 at 23 40 05@2x

If it looks roughly right, could you stamp this PR? I can handle further tweaks in a follow-up, but I want to use these notifications on other pages, and for that, it'd be nice to have these new designs, even if they're not final.

@vdavid vdavid force-pushed the dv/ssc-fix-notification-colors branch from 0ba8a09 to 495ad55 Compare May 23, 2024 21:44
@vdavid vdavid requested review from a team and taras-yemets May 24, 2024 07:24
@vdavid vdavid merged commit ebe14c4 into main May 24, 2024
@vdavid vdavid deleted the dv/ssc-fix-notification-colors branch May 24, 2024 07:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants