fix: styling for error messaging #647
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| @@ -0,0 +1,35 @@ | |||
| const FONT_SANS_SERIF = `-apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, 'Helvetica Neue', Helvetica, Arial, system-ui, sans-serif` | |||
There was a problem hiding this comment.
I stole these font choices from the uncaught error overlay in the sanity repo. I'm not sure if this is the best place for them but I put them here for now since it's the only styles file.
colepeters
left a comment
colepeters
left a comment
There was a problem hiding this comment.
Yaaaaas, first PR! Thanks Lauren! A few questions for discussion, but this looks great!
| heading="Before you continue..." | ||
| description="To access your content, you need to <b>add the following URL as a CORS origin</b> to your Sanity project." |
There was a problem hiding this comment.
Couple minor typographic/semantic nits (I know this code was inherited, all good!):
| heading="Before you continue..." | |
| description="To access your content, you need to <b>add the following URL as a CORS origin</b> to your Sanity project." | |
| heading="Before you continue…" | |
| description="To access your content, you need to <strong>add the following URL as a CORS origin</strong> to your Sanity project." |
There was a problem hiding this comment.
Feel free to adjust the wording in any way too.
| message?: string | ||
| } | ||
|
|
||
| export function Error({heading, description, code, cta, message}: ErrorProps): React.ReactNode { |
There was a problem hiding this comment.
I'm wondering if it would make sense for the description, code, and message props to be collapsed into a children prop, and just composing the necessary content with each instance where the Error component is used. This would mean having to import and use the styles wherever the component is used, but maybe that also keeps the component itself a little more flexible/reusable? What do you think? (Totally open ended on this!)
There was a problem hiding this comment.
I thought about that since those do feel like a lot of similar props but I also liked the idea that the component could be dropped in anywhere without having to think about its styling.
One thing I'm realizing as I test out the message field is that it repeats some of the information included in description and code and doesn't really flow all that well (also I'm wondering how often we'd even end up without a projectId and needing to use it). Maybe it would make sense to move that to the description and delete message altogther.
There was a problem hiding this comment.
Good points all around! Let's maybe converge some of this on description in that case as you suggest 👍🏻
There was a problem hiding this comment.
Just pushed up consolidating fields and the copy updates from above.
3 participants
Description
This PR adds basic styling to CORS and login error messaging. To keep the errors visually consistent, I did that by creating a new generic
Errorcomponent. I tried to avoid being too heavy handed with the styles since this component will also inherit any CSS in the parent app. For example, I didn't set colors since we can't know what background the user has chosen or if they're supporting light and dark modes. I've kept the link/button styling as simple as possible and I'm using pixels instead ofemorremsince we can't control the parent. I did set a font family because anything seemed better than Times New Roman.I also removed a bit of code introduced in this PR because, after talking with @ryanbonial, we realized the query error state wasn't updating and wasn't needed for rendering the CORS error.
What to review
Error.tsxandError.styles.tsfiles look okay. Open to any feedback on how I'm exporting inline styles or the props I'm passing to the component.LoginError.tsxandCorsErrorComponent.tsx, check that the correct props are being passed.Testing
Since these changes are mostly visual, I didn't add automated tests but did do manual testing. I looked at Kitchen Sink and the test SDK Movie Procurement app in light and dark modes and made sure the "Retry" and "Manage CORS Configuration" CTAs works as expected. Note, the Kitchen Sink app will also render an uncaught error overlay which I'm hiding in the screenshots below. I think it only happens there but we should keep an eye out.
To re-create the CORS error locally, I updated
vite.config.tsin the Kitchen Sink app with port3339and ranpnpm dev -- --port=3339. For the login error, I updated thegetTokenFromLocationfunction in the authutils.tsfile to return an invalid token.Fun gif