-
-
Notifications
You must be signed in to change notification settings - Fork 338
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
Handle API errors on schema page #2829
Changes from all commits
6e786a9
f56d5c5
35ab59f
c4c2b48
1cb4442
10200bb
bab4b39
8eb25a9
3e81b4b
3d6be61
b379939
39770f5
f51070e
2760918
25bb241
b01534d
41320ba
b78b154
85ca6f2
74fb1b0
cb20159
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,11 @@ | |
import type { Database, SchemaEntry } from '@mathesar/AppTypes'; | ||
import { AnchorButton } from '@mathesar-component-library'; | ||
import { getDataExplorerPageUrl } from '@mathesar/routes/urls'; | ||
import { refetchQueriesForSchema } from '@mathesar/stores/queries'; | ||
import { refetchTablesForSchema } from '@mathesar/stores/tables'; | ||
import { currentSchemaId } from '@mathesar/stores/schemas'; | ||
import { Button, Icon } from '@mathesar-component-library'; | ||
import { iconRefresh, iconWarning } from '@mathesar/icons'; | ||
import OverviewHeader from './OverviewHeader.svelte'; | ||
import TablesList from './TablesList.svelte'; | ||
import ExplorationsList from './ExplorationsList.svelte'; | ||
|
@@ -16,7 +21,9 @@ | |
export let tablesMap: Map<number, TableEntry>; | ||
export let explorationsMap: Map<number, QueryInstance>; | ||
export let isTablesLoading = false; | ||
export let isTablesUnfetchable = false; | ||
export let isExplorationsLoading = false; | ||
export let isExplorationsUnfetchable = false; | ||
|
||
export let canExecuteDDL: boolean; | ||
export let canEditMetadata: boolean; | ||
|
@@ -30,7 +37,11 @@ | |
$: showExplorationTutorial = hasTables && !hasExplorations && canEditMetadata; | ||
|
||
// Viewers can explore, they cannot save explorations | ||
$: canExplore = hasTables && hasExplorations && !isExplorationsLoading; | ||
$: canExplore = | ||
hasTables && | ||
hasExplorations && | ||
!isExplorationsLoading && | ||
!isExplorationsUnfetchable; | ||
</script> | ||
|
||
<div class="container"> | ||
|
@@ -44,6 +55,30 @@ | |
</OverviewHeader> | ||
{#if isTablesLoading} | ||
<TableSkeleton numTables={schema.num_tables} /> | ||
{:else if isTablesUnfetchable} | ||
<div class="table-fetch-error"> | ||
<div> | ||
<Icon {...iconWarning} /> | ||
<span>Error fetching tables</span> | ||
</div> | ||
<div> | ||
<Button | ||
on:click={() => { | ||
if ($currentSchemaId) { | ||
void refetchTablesForSchema($currentSchemaId); | ||
} | ||
}} | ||
> | ||
<Icon {...iconRefresh} /> | ||
<span>Retry</span> | ||
</Button> | ||
<a href="../"> | ||
<Button> | ||
<span>Go to Database</span> | ||
</Button> | ||
</a> | ||
</div> | ||
</div> | ||
{:else if showTableCreationTutorial} | ||
<CreateNewTableTutorial {database} {schema} /> | ||
{:else} | ||
|
@@ -60,6 +95,30 @@ | |
<OverviewHeader title="Saved Explorations" /> | ||
{#if isExplorationsLoading} | ||
<ExplorationSkeleton /> | ||
{:else if isExplorationsUnfetchable} | ||
<div class="exploration-fetch-error"> | ||
<div> | ||
<Icon {...iconWarning} /> | ||
<span>Error fetching explorations</span> | ||
</div> | ||
<div> | ||
<Button | ||
on:click={() => { | ||
if ($currentSchemaId) { | ||
void refetchQueriesForSchema($currentSchemaId); | ||
} | ||
}} | ||
> | ||
<Icon {...iconRefresh} /> | ||
<span>Retry</span> | ||
</Button> | ||
<a href="../"> | ||
<Button> | ||
<span>Go to Database</span> | ||
</Button> | ||
</a> | ||
</div> | ||
</div> | ||
Comment on lines
+99
to
+121
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should use ErrorBox component for this. This might lead to a minor difference in the UI as compared to the figma but that is fine |
||
{:else if showExplorationTutorial} | ||
<CreateNewExplorationTutorial {database} {schema} /> | ||
{:else} | ||
|
@@ -114,6 +173,45 @@ | |
} | ||
} | ||
|
||
.table-fetch-error { | ||
font-weight: 600; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
box-sizing: border-box; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is added to all elements by default in |
||
display: flex; | ||
flex-direction: column; | ||
justify-content: center; | ||
align-items: flex-start; | ||
padding: 16px; | ||
gap: 16px; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This does not have good browser support. You can use |
||
width: 616px; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please use |
||
height: 105px; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no need for a fixed |
||
background: #fdeeed; | ||
border: 1px solid #f9cdc8; | ||
Comment on lines
+187
to
+188
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please use css variables for colors. |
||
border-radius: 8px; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please use css variables for this. |
||
flex: none; | ||
order: 1; | ||
align-self: stretch; | ||
flex-grow: 0; | ||
} | ||
|
||
.exploration-fetch-error { | ||
font-weight: 600; | ||
box-sizing: border-box; | ||
display: flex; | ||
flex-direction: column; | ||
justify-content: center; | ||
align-items: flex-start; | ||
padding: 16px; | ||
gap: 16px; | ||
width: 304px; | ||
height: 105px; | ||
background: #fdeeed; | ||
border: 1px solid #f9cdc8; | ||
border-radius: 8px; | ||
flex: none; | ||
order: 1; | ||
align-self: stretch; | ||
} | ||
Comment on lines
+196
to
+213
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. look at the comments in the similar CSS above ( |
||
|
||
@media screen and (min-width: 64rem) { | ||
.container { | ||
flex-direction: row; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -59,7 +59,9 @@ | |
$: tablesMap = canExecuteDDL ? $tablesStore.data : $importVerifiedTablesStore; | ||
$: explorationsMap = $queries.data; | ||
$: isTablesLoading = $tablesStore.state === States.Loading; | ||
$: isTablesUnfetchable = $tablesStore.state === States.Error; | ||
$: isExplorationsLoading = $queries.requestStatus.state === 'processing'; | ||
$: isExplorationsUnfetchable = $queries.requestStatus.state === 'failure'; | ||
Comment on lines
61
to
+64
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. instead of passing separate variables for loading & error, please use |
||
|
||
$: tabs = [ | ||
{ | ||
|
@@ -154,7 +156,9 @@ | |
{canExecuteDDL} | ||
{canEditMetadata} | ||
{isTablesLoading} | ||
{isTablesUnfetchable} | ||
{isExplorationsLoading} | ||
{isExplorationsUnfetchable} | ||
{tablesMap} | ||
{explorationsMap} | ||
{database} | ||
|
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.
We should use
ErrorBox
component for this. This might lead to a minor difference in the UI as compared to the figma but that is fine