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: nested button error, refactor border radii, change receive modal #4559

Merged
merged 3 commits into from
Nov 22, 2023

Conversation

pete-watters
Copy link
Contributor

@pete-watters pete-watters commented Nov 22, 2023

Try out this version of Leather — download extension builds.

This PR has a few changes:

@@ -7,8 +7,12 @@ interface ReceiveItemListProps {
export function ReceiveItemList({ children, title }: ReceiveItemListProps) {
return (
<>
{title && <styled.span textStyle="">{title}</styled.span>}
<Divider mt="space.02" />
{title && (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mica000 - hopefully this looks OK now. The font style wasn't being set either

Screenshot 2023-11-22 at 09 23 03

@@ -18,6 +18,8 @@ export const tokens = defineTokens({
sm: { value: '10px' },
md: { value: '12px' },
lg: { value: '16px' },
xl: { value: '20px' },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a couple more border radius sizes. We can probably remove these one we determine the style we want.

Some were set as 50% / 100% so I changed these to sensible token sizes. The spinner for example was set to 100% but the same effect is achieved setting it to lg

The QR code was 18px but I set it to lg / 16px as I thought it looks OK and I didn't want to add tokens not divisible by 4.

@pete-watters
Copy link
Contributor Author

@mica000 @fabric-8 : I spent some time here refactoring the border-radius to use design tokens. This should make it easier of you want to try a few things out. You can also see the various sizes we have more easily.

tokens.ts

radii: {
    xs: { value: '8px' },
    sm: { value: '10px' },
    md: { value: '12px' },
    lg: { value: '16px' },
    xl: { value: '20px' },
    xxl: { value: '24px' },
  },

@pete-watters pete-watters added this pull request to the merge queue Nov 22, 2023
Merged via the queue into dev with commit 800fa3e Nov 22, 2023
26 checks passed
@pete-watters pete-watters deleted the fix/4550/the-hardest-button-to-button branch November 22, 2023 09:31
@mica000
Copy link

mica000 commented Nov 22, 2023

Thanks @pete-watters!!!!!

@fbwoolf
Copy link
Contributor

fbwoolf commented Nov 22, 2023

@pete-watters thanks for fixing, I may have accidentally done the nested button thing on accident when converting from as="button", maybe watch for it in other places just in case.

@pete-watters
Copy link
Contributor Author

@pete-watters thanks for fixing, I may have accidentally done the nested button thing on accident when converting from as="button", maybe watch for it in other places just in case.

No problem. Some small issues like this are bound to happen 👍 it was still working anyway, just never needed to be as button initially

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.

Button nested within button
4 participants