-
Notifications
You must be signed in to change notification settings - Fork 105
fix: redesign admin ui #3793
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
base: main
Are you sure you want to change the base?
fix: redesign admin ui #3793
Conversation
✅ Deploy Preview for brilliant-pasca-3e80ec ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
mkurapov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! I will ask the team for further reviews
| @@ -23,6 +33,60 @@ import type { ZodFieldErrors } from '~/shared/types' | |||
| import { capitalize, formatAmount } from '~/shared/utils' | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| import { capitalize, formatAmount } from '~/shared/utils' | |
| import { formatAmount } from '~/shared/utils' |
| </Flex> | ||
| </Flex> | ||
| <hr /> | ||
| in <Flex direction='column' gap='4'> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| in <Flex direction='column' gap='4'> | |
| <Flex direction='column' gap='4'> |
| className='absolute inset-0 bg-cover bg-center' | ||
| style={{ backgroundImage: `url(${bgUrl})` }} | ||
| /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can just keep the plain background
| className={`pt-20 md:pt-0 flex ${displaySidebar ? 'md:pl-60' : ''} flex-1 flex-col`} | ||
| > | ||
| <main className='pb-8 px-4'> | ||
| <body className='h-full text-tealish bg-diagonal'> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <body className='h-full text-tealish bg-diagonal'> | |
| <body className='h-full text-tealish'> |
Just personal preference :)
BlairCurrey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great overall. Much improved.
I will also +1 Max's suggestion on the background #3793 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not just specific to this file, but noticed it here.
If you search for <h1 or 2,3,4 you see we still use the raw html headings in places. Should we make all of those use radix <Heading>?
| <Card className='max-w-3xl'> | ||
| <Flex direction='column' gap='4'> | ||
| <Flex align='center' justify='between' gap='3' wrap='wrap'> | ||
| <Text className='rt-Text rt-r-size-2 rt-r-weight-medium uppercase tracking-wide text-gray-600 font-semibold'> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about making these section titles <Heading>s? Maybe a small thing but I think it's more semantic and matches what it was previously.
| className='h-full bg-polkadot bg-cover bg-no-repeat bg-center bg-fixed' | ||
| className='h-full' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.

Please go to the
Previewtab and select the appropriate sub-template: