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

refactor: rebrand SpotlightPopoverTour #1952

Merged
merged 6 commits into from
Jan 16, 2024

Conversation

snitin315
Copy link
Member

@snitin315 snitin315 commented Jan 11, 2024

  • No breaking changes.

Copy link

changeset-bot bot commented Jan 11, 2024

⚠️ No Changeset found

Latest commit: db76400

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

Copy link

codesandbox-ci bot commented Jan 11, 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 db76400:

Sandbox Source
razorpay/blade: basic Configuration

Copy link
Contributor

github-actions bot commented Jan 11, 2024

✅ PR title follows Conventional Commits specification.

@snitin315 snitin315 marked this pull request as ready for review January 11, 2024 05:37
@snitin315 snitin315 added the Review - L1 First level of review label Jan 11, 2024
@snitin315 snitin315 force-pushed the rebrand-spotlight-popover branch from df3d814 to 125968e Compare January 12, 2024 06:34
@snitin315 snitin315 added Review - L2 Second level of review and removed Review - L1 First level of review labels Jan 12, 2024
@@ -110,12 +110,12 @@ export const BasicExample = `
>
<Box width="100%" display="flex" gap="spacing.4">
<SpotlightPopoverTourStep name="step-1">
<Box padding="spacing.4" backgroundColor="brand.gray.400.lowContrast">
<Box padding="spacing.4" backgroundColor="interactive.border.gray.faded">
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are we using interactive color on a box? should be surface right?

Copy link
Member Author

@snitin315 snitin315 Jan 15, 2024

Choose a reason for hiding this comment

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

Hmm, this value is mapped in the codemod. I think we should update this with surface.border.gray.muted.

"brand.gray.400.lowContrast": "interactive.border.gray.faded",

Screenshot 2024-01-15 at 3 48 16 PM

interactive.border.gray.faded & surface.border.gray.muted have the same value.

muted: globalColors.neutral.blueGrayLight.a100,

faded: globalColors.neutral.blueGrayLight.a100,

Copy link
Member

Choose a reason for hiding this comment

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

@snitin315 put this on a TODO somewhere, we will take this up as a separate PR.
Let's merge this component. This is just an example file.

// cc @kamleshchandnani

Copy link
Collaborator

Choose a reason for hiding this comment

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

why separate PR? one line change hai lets do it?

Copy link
Member

@anuraghazra anuraghazra Jan 16, 2024

Choose a reason for hiding this comment

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

Are we sure about this token? We are setting border color in backgroundColor.

And I'm not sure we should also map this to border on the colorMapping.json file - since we don't know if it's being used as border/background or surface.

Copy link
Member Author

@snitin315 snitin315 Jan 16, 2024

Choose a reason for hiding this comment

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

This was previously discussed in #1883 (comment)

So currently, we migrate every token to surface tokens by default via the colorMapping.json , then we try to update these tokens contextually based on usage wherever possible.

// Update color token value based on the context

it('should update token values contextually', async () => {

However, the issue with brand.gray.* tokens is that not all the tokens have their equivalent in surface tokens. hence we have them mapped to border.*. So we need designers inputs here.

@kamleshchandnani @anuraghazra

@kamleshchandnani kamleshchandnani merged commit 1dd8b43 into rebranding/master Jan 16, 2024
13 of 15 checks passed
@kamleshchandnani kamleshchandnani deleted the rebrand-spotlight-popover 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