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
refactor(field): migrate layout components to sanity ui - phase 1 #2546
Conversation
This pull request is being automatically deployed with Vercel (learn more). test-studio – ./🔍 Inspect: https://vercel.com/sanity-io/test-studio/CsTR96JLbKvs8Lrv5qCruDbN3bpo perf-studio – ./🔍 Inspect: https://vercel.com/sanity-io/perf-studio/6nu3kHtytq9CYzg4FUrpPGssf4hG |
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.
Really nice work here! I only have concerns about RevertIcon
and ErrorOutlineIcon
. The other ones are optional nitpicks and praise 🙌
packages/@sanity/field/src/diff/components/DiffErrorBoundary.tsx
Outdated
Show resolved
Hide resolved
The component responsible for showing the changes to this field has crashed. | ||
</Text> | ||
{help && ( | ||
<Text size={1} as="p"> |
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.
Nitpick: I think it would look good to use muted
on this Text
. So the headline is a little darker than the copy below it.
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.
Good thinking!
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.
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.
Agreed, but let’s fix the theme (in another PR, as mentioned in catch-up)
} | ||
|
||
interface DiffTooltipWithAnnotationsProps { | ||
interface DiffTooltipWithAnnotationsProps extends TooltipProps { |
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.
Nice detail to extend the TooltipProps
here 👍
backgroundColor: color.background, | ||
color: color.text, | ||
borderRadius: 'calc(23px / 2)', | ||
}} |
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.
Nitpick (perhaps for a separate PR): I believe we could make this more reusable by creating a styled component here. Something like:
const UserColoredCard = styled(Card)<{$color: {background: string, text: string}}>`
--card-bg-color: ${props => props.$color.background};
--card-fg-color: ${props => props.$color.text};
--card-muted-fg-color: ${props => props.$color.text};
`
And using it like:
<UserColoredCard $color={color} style={{borderRadius: 'calc(23px / 2)'}}>
...
</UserColoredCard>
</Text> | ||
</Box> | ||
)} | ||
<Text size={1} weight="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.
You did correct by converting font-weight: 600
to semibold
, but actually medium
would be best so the these headers look similar to the field headers in the form.
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.
the field headers are also using semibold tho? 🤔
<Text as="label" htmlFor={inputId} weight="semibold" size={1}> |
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.
Ah true, then why do they look lighter ... 🤔
8df39e8
to
bc1f777
Compare
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.
LGTM ✨
I figured it out! It was applied in another component (with |
Do not merge before AFTER the next release 👍 |
bc1f777
to
b72710e
Compare
b72710e
to
f5c3179
Compare
f5c3179
to
d3a4670
Compare
NOTE: Do not merge before AFTER the next release 👍
Description
First phase of migrating components in @sanity/field to sanity ui and styled-components.
These are smaller changes with minimal visual changes.
I did not touch the PopoverDialog in the change list yet, will do that in another phase.
What to review
Make sure that things still look ok visually in the changes panel. I've attached screenshots for an overview.
Error boundary
Before
After
Change list, tooltip, breadcrumbs
Before (left) and after (right)
Notes for release