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

Permit styles [Modal/Stepper/IconSpinner] #3097

Merged
merged 8 commits into from
Sep 7, 2023

Conversation

fairlighteth
Copy link
Contributor

Summary

Adds updated Modal Stepper and IconSpinner modules. To be used with the Permit flow. Available on Cosmos.

Screen.Recording.2023-08-31.at.15.07.16.mov
Screenshot 2023-08-31 at 15 06 35
Screen.Recording.2023-08-31.at.15.06.20.mov
Screen.Recording.2023-08-31.at.15.06.02.mov

@vercel
Copy link

vercel bot commented Aug 31, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
swap-dev ❌ Failed (Inspect) Sep 4, 2023 5:26pm

@fairlighteth fairlighteth requested a review from a team August 31, 2023 14:09
content = <span />;
}

return (
Copy link
Collaborator

@shoom3301 shoom3301 Sep 1, 2023

Choose a reason for hiding this comment

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

Just an idea how to write it shorter:

<Wrapper size={size} spinnerWidth={spinnerWidth} bgColor={bgColor}>
      {(() => {
        if (currency) {
          return <CurrencyLogo currency={currency} size="100%" />
        } else if (image) {
          return <img src={image} alt="Spinning icon" width={size} height={size} />
        } else if (children) {
          return <span>{children}</span>
        }

        return <span />
      })()}
    </Wrapper>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the suggestion, updated!

dotBorderColor?: string
}

const stateStyles = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, specify a type for the const:

interface StepStyles {
  dotColor: string
  dotBorderColor: string
  labelColor: string
}

const stateStyles: Record<StepState, StepStyles> = {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated!

* feat: optimize IconSpinner

* feat: optimize

* feat: add ENUM
@alfetopito
Copy link
Collaborator

Something is wrong with the types, the build is failing

image

Copy link
Collaborator

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

Leaving some example of usage of the enum for variable names.

margin: auto;

> path {
fill: var(--cow-color-text2);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here the enum would be used:

Suggested change
fill: var(--cow-color-text2);
fill: var(${ColorTypes.COLOR_TEXT_2});

Comment on lines +156 to +158
background: var(--cow-container-bg-01);
border-radius: var(--cow-border-radius-normal);
box-shadow: var(--cow-box-shadow-normal);
Copy link
Collaborator

Choose a reason for hiding this comment

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

And here

Suggested change
background: var(--cow-container-bg-01);
border-radius: var(--cow-border-radius-normal);
box-shadow: var(--cow-box-shadow-normal);
background: var(${ColorTypes.CONTAINER_BG_01});
border-radius: var(${ColorTypes.BORDER_RADIUS_NORMAL});
box-shadow: var(${ColorTypes.BOX_SHADOW_NORMAL});

And so on

Comment on lines +404 to +409
--cow-color-border: var(--cow-color-grey);
--cow-container-bg-01: ${({ theme }) => theme.bg1};
--cow-modal-backdrop: var(--cow-color-text1-opacity-25);
--cow-border-radius-normal: 24px;
--cow-padding-normal: 24px;
--cow-box-shadow-normal: 0 1.5rem 1rem #00000008,0 .75rem .75rem #0000000a,0 .25rem .375rem #0000000d;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here is a base place for their definition, using the same way.

It's just a template string, so it shouldn't add any extra processing/make it slower

Suggested change
--cow-color-border: var(--cow-color-grey);
--cow-container-bg-01: ${({ theme }) => theme.bg1};
--cow-modal-backdrop: var(--cow-color-text1-opacity-25);
--cow-border-radius-normal: 24px;
--cow-padding-normal: 24px;
--cow-box-shadow-normal: 0 1.5rem 1rem #00000008,0 .75rem .75rem #0000000a,0 .25rem .375rem #0000000d;
${ColorTypes.COLOR_BORDER}: var(${ColorTypes.COLOR_GREY});
--cow-container-bg-01: ${({ theme }) => theme.bg1};
--cow-modal-backdrop: var(--cow-color-text1-opacity-25);
--cow-border-radius-normal: 24px;
--cow-padding-normal: 24px;
--cow-box-shadow-normal: 0 1.5rem 1rem #00000008,0 .75rem .75rem #0000000a,0 .25rem .375rem #0000000d;

@fairlighteth
Copy link
Contributor Author

Merging as I'm addressing the ENUMs in #3110

@fairlighteth fairlighteth merged commit 86d2637 into permit-styles-tag-1 Sep 7, 2023
6 of 7 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 7, 2023
@alfetopito alfetopito deleted the permit-styles-modal-1 branch September 8, 2023 10:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants