fix(site): ensure Error Boundary catches render errors correctly #15963
Conversation
| type={ | ||
| props.type === undefined && Comp === "button" ? "button" : props.type | ||
| } |
There was a problem hiding this comment.
Small change to make sure that you have to opt into submit behavior for true buttons
There was a problem hiding this comment.
I’m not sure I fully understand the value or use case for this 🤔. Could you share an example to help clarify? At first glance, it seems like the type prop should be the consumer’s responsibility rather than something built into the component.
There was a problem hiding this comment.
So, I feel that +90% of the time, when you have a <button>, you want it to be type="button" instead of the default type="submit", and even experienced frontend engineers can forget to do that. That sometimes leads to forms accidentally getting submitted when you didn't mean for them to. I think it's a more reasonable default value that can't ever be made official because of HTML backwards-compatibility
And since the Radix button component is polymorphic, we don't have any linting rules to catch that you didn't set the type, but also because it's polymorphic, the final output might not be an actual button. If we use asChild to style a link like a button, we can't always assume that we can attach a type attribute. <a> doesn't support the attribute, so while React would let us add it no problem, the HTML would be invalid
So basically, the check makes it so that buttons will default to type="button" if the output would be a button, and if the prop wasn't already set. Otherwise, yeah, it does fall to the user to wire things up correctly
There was a problem hiding this comment.
Do you think it would make sense to summarize this decision and add it as a comment? I’m concerned we might forget this decision in the future and accidentally remove it, as it’s not a very common or usual approach.
| ref={ref} | ||
| {...props} | ||
| className={cn(buttonVariants({ variant, size }), className)} |
There was a problem hiding this comment.
We had a typo where we were accidentally passing the consumer class name into CVA, instead of passing it directly to cn
| </svg> | ||
|
|
||
| <div className="text-content-primary flex flex-col gap-1"> | ||
| <h1 className="text-2xl font-normal m-0">{errorPageTitle}</h1> |
There was a problem hiding this comment.
@BrunoQuaresma I was wondering about this: I swear Tailwind's preflight styles get rid of all margins, period. So I wasn't sure why I as still getting them for headers and paragraphs
I saw a few other components using this, but I wasn't sure if we had to override Tailwind to get it to work with our current MUI setup
There was a problem hiding this comment.
Currently, we’ve disabled Tailwind’s preflight option to simplify integration with MUI. Hopefully, we can enable it again soon.
References:
| <rect width="68" height="49" /> | ||
| </clipPath> | ||
| </defs> | ||
| </svg> |
There was a problem hiding this comment.
Nit: Since you’re already working on this part of the code, maybe we could replace this long SVG with the Coder logo component we use for the navbar. What do you think?
There was a problem hiding this comment.
No, not a nit at all. This component was originally code for the registry that I repurposed, and I forgot to see if we had an existing logo we could swap in
I'll get that fixed
|
Awesome work, and thanks so much for getting this fixed 🙏. The only blocker for me is the decision in the |
No issue or design to link to. Squeezing this in between other stuff
Changes made
ErrorBoundaryfunctionality withGlobalErrorBoundarycomponentGlobalErrorBoundaryto React Router rather than the top of the appScreenshots
Before
After
Notes