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 border-radius #255

Merged
merged 3 commits into from
Jul 17, 2023
Merged

fix border-radius #255

merged 3 commits into from
Jul 17, 2023

Conversation

Lykhoyda
Copy link
Collaborator

@Lykhoyda Lykhoyda commented Jul 14, 2023

Closes #253

Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

Thanks, it def looks more consistent that way.
There's just a little thing that I made a suggestion for. If that work I think it's better, if not we can forget as this will be changed eventually.

@@ -141,7 +141,7 @@ export default styled(Transaction)(
max-height: 1.3125rem;
left: 29px;
top: 19px;
border-radius: 0 0.9375rem 0.9375rem 0.9375rem;
border-radius: ${theme.custom.borderRadius};
Copy link
Collaborator

Choose a reason for hiding this comment

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

This breaks the effect of having the top left not having any in the top left. Not sure if this suggestion works, we we could try

Suggested change
border-radius: ${theme.custom.borderRadius};
border-radius: 0 ${theme.custom.borderRadius} ${theme.custom.borderRadius} ${theme.custom.borderRadius};

Copy link
Collaborator Author

@Lykhoyda Lykhoyda Jul 14, 2023

Choose a reason for hiding this comment

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

The new design doesn't have that view for the badge. So we can consider it deprecated and keep the new design

Copy link
Collaborator

Choose a reason for hiding this comment

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

this is not implementing the new design though, we're in the old, changing border-radius. Now if that's something you've strong opinion about, because you don't like it, I can let it go.

@Tbaut
Copy link
Collaborator

Tbaut commented Jul 15, 2023

I guess it's ready to merge?

@Lykhoyda
Copy link
Collaborator Author

I guess it's ready to merge?

Now we can merge, got the reply from Cindy about unified border-radius

@Lykhoyda Lykhoyda merged commit 7851e73 into main Jul 17, 2023
5 checks passed
@Lykhoyda Lykhoyda deleted the lykhoyda/fix_border-radius branch July 17, 2023 09:38
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.

Unify border radius
2 participants