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: design qa feedback #1033

Merged
merged 14 commits into from
Oct 10, 2024
Merged

fix: design qa feedback #1033

merged 14 commits into from
Oct 10, 2024

Conversation

RobChangCA
Copy link
Collaborator

@RobChangCA RobChangCA commented Oct 9, 2024

Pull Request Checklist


PR-Codex overview

This PR focuses on updating styles and class names across various components for improved UI consistency and responsiveness. It also includes minor text adjustments for clarity.

Detailed summary

  • Updated className properties for various components to improve styling.
  • Adjusted spacing in several components for better layout.
  • Changed text in NFT.tsx and other components for clarity.
  • Refined div elements and their attributes for consistency.
  • Enhanced Switch component styling for better visibility.

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

Copy link

vercel bot commented Oct 9, 2024

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

Name Status Preview Comments Updated (UTC)
aa-sdk-site ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 10, 2024 7:46pm
aa-sdk-ui-demo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 10, 2024 7:46pm

@@ -12,7 +12,7 @@ const CodePreviewSwitch = React.forwardRef<
>(({ className, ...props }, ref) => (
<SwitchPrimitives.Root
className={cn(
"peer inline-flex h-[28px] w-[50px] shrink-0 cursor-pointer items-center rounded-full border-2 border-transparent transition-colors focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 focus-visible:ring-offset-background disabled:cursor-not-allowed disabled:opacity-50 data-[state=checked]:bg-brand data-[state=unchecked]:bg-[#94A3B8]",
"peer inline-flex h-[28px] w-[50px] shrink-0 cursor-pointer items-center rounded-full border-2 border-transparent transition-colors focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 focus-visible:ring-offset-background disabled:cursor-not-allowed disabled:opacity-50 data-[state=checked]:bg-[#020617] data-[state=unchecked]:bg-[#94A3B8]",
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: can we put these bg colors into the theme for the ui-demo somewhere? I ran into some inconsistencies with background colors on the toggles (eg. Email toggle vs Passkey On Signup toggle when passkeys are off)

There's also a thread about revisiting some of the bg colors of the toggles and that will make it easier to address them all

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are hardcoded because we don't want them to change when we alter the theme from light to dark

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we end up theming the sidebar we will refactor all the hardcoded values.

>
<NFT
nftTransfered={nftTransfered}
transactionUrl={transactionUrl}
status={status}
/>
<MintCardActionButtons
className="hidden xl:block"
className="hidden xl:block w-[208px] m-auto"
Copy link
Collaborator

Choose a reason for hiding this comment

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

any reason for the w of this to be controlled by flex-1 and use padding / break points to control the spacing and layout?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

they want it to match the width of the mint image which isn't a direct sibling

Copy link
Collaborator

@moldy530 moldy530 left a comment

Choose a reason for hiding this comment

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

generally looks good to me, but we need to move these hardcoded hex values into some central, reusable location incase things change in the future. I know we have a ticket to handle that separately, so will not block this on that

@RobChangCA RobChangCA merged commit c00381a into main Oct 10, 2024
6 checks passed
@RobChangCA RobChangCA deleted the rob/qa branch October 10, 2024 20:03
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.

2 participants