-
Notifications
You must be signed in to change notification settings - Fork 0
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
fix: better error and loading handling on programs page #1012
base: main
Are you sure you want to change the base?
Conversation
45c2a57
to
1678860
Compare
Unless I'm misunderstanding something, I'm afraid we may need to disable It's not enabled in the upstream project, either: https://github.com/adobe/react-spectrum/blob/main/tsconfig.json |
I would have assumed that the compiler options we set for our package are not passed to dependencies, since the creators of a particular library may intentionally ignore particular options. Is this not the case? Anyway, I wouldn't actually mind too much about disabling this particular option. It can be quite annoying differentiating between |
I'm also confused about that, but it appears not. Maybe I've misconfigured something.
I feel similarly, so I'll just disable it in this PR, then. |
If I enable this option: Line 12 in a1709ab
then the problem goes away. It's documented here. It seems like this alternative solution is preferred to skipping the lib check, however. |
I've tried a few different https://github.com/adobe/react-spectrum/blob/8c9d3bdc279568e76e406adc924607c04d796009/tsconfig.json#L6 So I think I'll just disable |
497bef6
to
5becb05
Compare
4c5f047
to
bf3f94a
Compare
bf3f94a
to
11cc2d4
Compare
The error and loading messages are just placeholders for now. The crucial change here is to let the student know when we can't display up-to-date session information for some reason. Note that we show data if it's available, without first checking for `isLoading` or `isError`. This means we may show stale data, but we prefer this over showing a loading message or an error for short server outages. For some justification of this approach, see: https://tkdodo.eu/blog/status-checks-in-react-query Note that React Query will not show stale data indefinitely, and will eventually show an error message if the data is stale for too long. Signed-off-by: Drew Hess <[email protected]>
The previous commit improved matters on the session page by handling `isError` and `isLoading` conditions, but has annoying behavior when the student is typing a program name into the search bar and we're doing live search, because the sessions page was rendered all-or-nothing. Now we always show the nav bar. This means we no longer have a nice single component for the sessions page (i.e., the `SessionsPage` component is no more), but `ChooseSession` is now more or less the same thing, only with better error and loading behavior. I haven't bothered to add a Storybook story for it, but it would be easyenough to add later if we need it. Signed-off-by: Drew Hess <[email protected]>
Signed-off-by: Drew Hess <[email protected]>
This setting is causing problems with one of `react-aria`'s dependencies, and we've found it to be probably more annoying than it's worth anyway, on balance. Signed-off-by: Drew Hess <[email protected]>
Signed-off-by: Drew Hess <[email protected]>
Signed-off-by: Drew Hess <[email protected]>
11cc2d4
to
4f489a6
Compare
This setting is causing problems for us with one package we'd like to use: #1012 (comment) and is also preventing us from migrating to TypeScript 5 due to an upstream issue in `react-router` that's been unaddressed for almost a year: #898 There were no objections to disabling it, so we do so in this commit. (There is one minor code change required as a result, just an additional optional property check on a `onNodeClick` handler in our React Flow wrapper.) Signed-off-by: Drew Hess <[email protected]>
This setting is causing problems for us with one package we'd like to use: #1012 (comment) and is also preventing us from migrating to TypeScript 5 due to an upstream issue in `react-router` that's been unaddressed for almost a year: #898 There were no objections to disabling it, so we do so in this commit. (There is one minor code change required as a result, just an additional optional property check on a `onNodeClick` handler in our React Flow wrapper.) Closes #898 Signed-off-by: Drew Hess <[email protected]>
I've disabled |
No description provided.