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

feat(blade): rebranding whitelabel fix #1959

Merged
merged 14 commits into from
Jan 16, 2024

Conversation

anuraghazra
Copy link
Member

No description provided.

Copy link

changeset-bot bot commented Jan 11, 2024

⚠️ No Changeset found

Latest commit: 9cb5617

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@anuraghazra anuraghazra changed the base branch from master to rebranding/master January 11, 2024 11:23
Copy link
Contributor

github-actions bot commented Jan 11, 2024

✅ PR title follows Conventional Commits specification.

Copy link

codesandbox-ci bot commented Jan 12, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 9cb5617:

Sandbox Source
razorpay/blade: basic Configuration

@anuraghazra anuraghazra added the Review - L1 First level of review label Jan 12, 2024
@chaitanyadeorukhkar
Copy link
Collaborator

Lets use the primary color on carousel's quotes like we do on master? Right now it always remains blue
Screenshot 2024-01-12 at 12 58 03 PM

@chaitanyadeorukhkar
Copy link
Collaborator

Focus ring color is not changing for multiple components like Checkbox, Radio, Accordion, Switch, Chips... can you check this once? I created a common getFocusRingStyles function that uses surface.border.primary.muted which should ideally change as per the white labeled theme
Screenshot 2024-01-12 at 1 00 25 PM

@chaitanyadeorukhkar
Copy link
Collaborator

Carousel's button's arrow icons should also follow the primary color, right?
Screenshot 2024-01-12 at 1 03 22 PM

@anuraghazra
Copy link
Member Author

Focus ring color

Now that you mention it, I wonder if we should keep the focus ring color static and not tie it with brand color.

The focus ring is an fundamental accessibility requirement, if brand color becomes very light then focus ring color won't be visible to the user and could cause accessibility issues.

https://www.w3.org/WAI/WCAG22/Understanding/focus-appearance.html

@anuraghazra
Copy link
Member Author

Carousel's button's arrow icons should also follow the primary color, right?

Nope, rebranded carousel nav buttons icons are staticBlack.

@anuraghazra
Copy link
Member Author

Lets use the primary color on carousel's quotes like we do on master? Right now it always remains blue

Done.

@chaitanyadeorukhkar
Copy link
Collaborator

Focus ring color

Now that you mention it, I wonder if we should keep the focus ring color static and not tie it with brand color.

The focus ring is an fundamental accessibility requirement, if brand color becomes very light then focus ring color won't be visible to the user and could cause accessibility issues.

https://www.w3.org/WAI/WCAG22/Understanding/focus-appearance.html

Hmm. That is fair. I think that we can think about on its own but right now we're using primary color token for the focus ring ideally, it should use the primary color when it changes as well?

@chaitanyadeorukhkar
Copy link
Collaborator

Carousel's button's arrow icons should also follow the primary color, right?

Nope, rebranded carousel nav buttons icons are staticBlack.

Why are they blue in this screenshot then? 🤔
Screenshot 2024-01-12 at 1 03 22 PM

@chaitanyadeorukhkar
Copy link
Collaborator

@anuraghazra Tests are failing. Mostly looks good to me. Just need to check

  1. Focus ring color
  2. Carousel's blue button

@anuraghazra
Copy link
Member Author

Why are they blue in this screenshot then? 🤔

Wierd, are you checking the correct chromati link? Because it's not possible for it to be blue.

image

@chaitanyadeorukhkar
Copy link
Collaborator

Why are they blue in this screenshot then? 🤔

Wierd, are you checking the correct chromati link? Because it's not possible for it to be blue.

Might be the wrong link. Checked again. Looks good

@anuraghazra
Copy link
Member Author

Focus ring color

I've standardized all components to use the getFocusRingColor function.

@anuraghazra anuraghazra added Review - L2 Second level of review and removed Review - L1 First level of review labels Jan 16, 2024
@kamleshchandnani kamleshchandnani merged commit 856f56d into rebranding/master Jan 16, 2024
13 of 15 checks passed
@kamleshchandnani kamleshchandnani deleted the anu/rebranding-whitelabel-fix branch January 16, 2024 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review - L2 Second level of review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants