🔨 Switch CodeSadbox to useOvermind, Remove title, add label to Report #2498
Conversation
Small refactor to make the switch to Overmind. Removed the title from the generated crash report, which will now require users to enter their own in order to be able to submit the report. Hopefully this makes crash reports more descriptive. Crash Reports now include the proper label automatically when they are generated. @christianalfoni there appears to be an issue with Overmind in this component, as isLoggedIn returns false even when the user is in fact logged in (test this by running locally and navigating to http://localhost:3000/codesadbox from another page). Not quite sure how to solve this one, so I'd appreciate it if you can investigate! Probably has something to do with the error boundary.
|
This pull request is automatically deployed with Now. Latest deployment for this branch: https://codesandbox-client-git-convert-codesadbox-to-overmind.codesandbox1.now.sh |
Saeris
added
the
🧠 Overmind
|
Sorry, I misunderstood the question on my deleted response :) Yeah, initially that made sense to me as well :) But it is a class and needs the But after putting it into "codesadbox" I think it makes more sense from the perspective of it being an actual route. The loading of the initial data and user is related to hitting routes, so if we later refactor to a "state first routing" approach this will still work. The |
| rel="noopener" | ||
| href={buildCrashReport({ error, trace })} | ||
| > | ||
| export const CodeSadbox: React.FC<IFallbackComponentProps> = ({ |
There was a problem hiding this comment.
I think using FunctionComponent would make the type a bit more clear.
| export const CodeSadbox: React.FC<IFallbackComponentProps> = ({ | |
| export const CodeSadbox: React.FunctionComponent<IFallbackComponentProps> = ({ |
There was a problem hiding this comment.
Yeah, I am a bit split on this. I started using FunctionComponent, but went for FC due to lazyness. Totally agree it is more explicit, though this type is not really what tells us this is a component. It is the folder name, file name and not at least the JSX in the file. We would never like... "what is this thing? Oh, let me check the type... hm React.FC... hmmmm... oh, it is a component", you know long before you look at the type that it is a component 😄
…sandbox#2498) * 🔨 Switch CodeSadbox to useOvermind, Remove title, add label to Report Small refactor to make the switch to Overmind. Removed the title from the generated crash report, which will now require users to enter their own in order to be able to submit the report. Hopefully this makes crash reports more descriptive. Crash Reports now include the proper label automatically when they are generated. @christianalfoni there appears to be an issue with Overmind in this component, as isLoggedIn returns false even when the user is in fact logged in (test this by running locally and navigating to http://localhost:3000/codesadbox from another page). Not quite sure how to solve this one, so I'd appreciate it if you can investigate! Probably has something to do with the error boundary. * fix mounting logic for codesadbox * fix types
Small refactor to make the switch to Overmind.
Removed the title from the generated crash report, which will now require users to enter their own in order to be able to submit the report. Hopefully this makes crash reports more descriptive. Crash Reports now include the proper label automatically when they are generated.
@christianalfoni there appears to be an issue with Overmind in this component, as isLoggedIn returns false even when the user is in fact logged in (test this by running locally and navigating to http://localhost:3000/codesadbox from another page). Not quite sure how to solve this one, so I'd appreciate it if you can investigate! Probably has something to do with the error boundary.