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

fix: ButtonGroup focus ring styles #2091

Merged
merged 2 commits into from
Mar 22, 2024
Merged

fix: ButtonGroup focus ring styles #2091

merged 2 commits into from
Mar 22, 2024

Conversation

snitin315
Copy link
Member

Description

Fixed the focus ring styles.

Changes

Screen.Recording.2024-03-20.at.12.48.32.PM.mov

Copy link

changeset-bot bot commented Mar 20, 2024

⚠️ No Changeset found

Latest commit: e762c56

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
Contributor

github-actions bot commented Mar 20, 2024

✅ PR title follows Conventional Commits specification.

@rzpcibot
Copy link
Collaborator

rzpcibot commented Mar 20, 2024

Bundle Size Report

Updated Components
Status Component Base Size (kb) Current Size (kb) Diff
ButtonGroup 1.171 1.154 -0.017 KB

Generated by 🚫 dangerJS against e762c56

Copy link

codesandbox-ci bot commented Mar 20, 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 e762c56:

Sandbox Source
razorpay/blade: basic Configuration

@snitin315 snitin315 added the Review - L1 First level of review label Mar 20, 2024
borderColor: getIn(
theme.colors,
getBackgroundColorToken({
// Only secondary variant has border a border, for other variants we use background color so that the border is not visible
Copy link
Member

Choose a reason for hiding this comment

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

Why not make it transparent?

Copy link
Member Author

@snitin315 snitin315 Mar 20, 2024

Choose a reason for hiding this comment

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

transparent doesn't work. BorderRadius is not getting applied.

Screenshot 2024-03-20 at 4 07 12 PM

@@ -10,22 +10,19 @@ const StyledButtonGroup = styled(BaseBox)<StyledButtonGroupProps>(
return {
display: 'flex',
width: isFullWidth ? '100%' : 'fit-content',
overflow: 'hidden',
Copy link
Member

Choose a reason for hiding this comment

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

was this causing issue with focus ring? (storybook is broken right now so unable to preview before and after https://razorpay.slack.com/archives/G01B3LQ9H0W/p1710930648131329)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.

@snitin315 snitin315 force-pushed the fix-buttongroup-focus branch from a931144 to e762c56 Compare March 21, 2024 05:09
@snitin315 snitin315 merged commit 3d63292 into master Mar 22, 2024
16 of 17 checks passed
@snitin315 snitin315 deleted the fix-buttongroup-focus branch March 22, 2024 09:42
@snitin315 snitin315 removed the Review - L1 First level of review label Mar 22, 2024
anuraghazra pushed a commit that referenced this pull request Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants