Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WEB-3979] 14.5.2 - enhanced typings for icons and colours, updated neutral colours #475

Merged
merged 1 commit into from
Sep 11, 2024

Conversation

jamiehenson
Copy link
Member

Jira Ticket Link / Motivation

The typing work is a side-chore (WEB-3979), the colour updates are https://ably.atlassian.net/browse/WEB-3987

Summary of changes

The colour updates are fairly simple, neutral-100 to neutral-500 have been updated with new values.

The typing work consists of generating union types for all the possible icon names and Tailwind colour classes, so that we can add further guards around props that use them - this PR adds them as such.

For example, on the Icon component, the value we pass into name can no longer just be a string, it has to match a list of icons that we actually have - this prevents accidental errors and highlights gaps in ably-ui if we need to add new icons.

How do you manually test this?

It's a behind the scenes change, so pull it down and mess with some icon name and colour props, I guess.

Reviewer Tasks (optional)

Merge/Deploy Checklist

  • Written automated tests for implemented features/fixed bugs
  • Rebased and squashed commits
  • Commits have clear descriptions of their changes
  • Checked for any performance regressions

Frontend Checklist

  • No frontend changes in this PR
  • Added before/after screenshots for changes
  • Tested on different platforms/browsers with Browserstack
  • Compared with the initial design / our brand guidelines
  • Checked the code for accessibility issues (VoiceOver User Guide)?

@@ -0,0 +1,101 @@
import React from "react";
Copy link
Member Author

@jamiehenson jamiehenson Sep 11, 2024

Choose a reason for hiding this comment

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

This file has mainly just been moved, just changed the arguments to iconSet

Copy link
Contributor

@aralovelace aralovelace left a comment

Choose a reason for hiding this comment

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

Nice! just have 1 question

I will just add this change once this PR goes live

@@ -0,0 +1,196 @@
export type IconName =
Copy link
Contributor

Choose a reason for hiding this comment

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

This is incredible! Neatly done!

--color-neutral-300: #edf1f7;
--color-neutral-100: #f6f8fa;
--color-neutral-200: #eef1f6;
--color-neutral-300: #e6eaf0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to check the pages that affect these changes? or not a lot of difference between these colors?

Copy link
Member Author

Choose a reason for hiding this comment

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

Pretty subtle changes - we can just roll this out

@jamiehenson jamiehenson merged commit 00ea724 into main Sep 11, 2024
3 checks passed
@jamiehenson jamiehenson deleted the 14.5.2 branch September 11, 2024 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants