Handle API errors on schema page #2829
Conversation
|
Hi, @rajatvijay, this PR needs to be modified accordingly once the design is available. Did I address the problem correctly? Let me know your views on this. |
Aritra8438
changed the title
|
@Aritra8438 I am assigning this to myself until I reply to you back with the design questions that you asked. |
rajatvijay
added
needs: unblocking
|
This is blocked on the work design, recorded under the concerned issue. |
ghislaineguerin
assigned ghislaineguerin and Aritra8438 and unassigned Aritra8438 and ghislaineguerin
|
@Aritra8438 I have updated the description to include the design. It also contains a link to the figma file |
rajatvijay
added
pr-status: revision
|
Hi, @rajatvijay. I have completed adding the design. |
|
@Aritra8438 thanks, I will review this between today and tomorrow. |
|
This pull request has not been updated in 45 days and is being marked as stale. It will automatically be closed in 30 days if not updated by then. |
|
@rajatvijay! A friendly ping. |
There was a problem hiding this comment.
@Aritra8438 Apologies for the delay. I have added some review comments, let me know if you need more clarity on any of them.
| <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> |
There was a problem hiding this comment.
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
| <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> |
There was a problem hiding this comment.
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
| justify-content: center; | ||
| align-items: flex-start; | ||
| padding: 16px; | ||
| gap: 16px; |
There was a problem hiding this comment.
This does not have good browser support. You can use :global(* + *). You can also do a global search for this code piece to see how it's being used at other places.
| @@ -114,6 +173,45 @@ | |||
| } | |||
| } | |||
|
|
|||
| .table-fetch-error { | |||
| font-weight: 600; | |||
| box-sizing: border-box; | |||
There was a problem hiding this comment.
This is added to all elements by default in base.html no need here again.
| @@ -114,6 +173,45 @@ | |||
| } | |||
| } | |||
|
|
|||
| .table-fetch-error { | |||
| font-weight: 600; | |||
There was a problem hiding this comment.
This makes it look unnecessarily bold(I have removed it from the left box in the SS). font-weight can be omitted completely.
| padding: 16px; | ||
| gap: 16px; | ||
| width: 616px; | ||
| height: 105px; |
There was a problem hiding this comment.
no need for a fixed height
| background: #fdeeed; | ||
| border: 1px solid #f9cdc8; |
There was a problem hiding this comment.
please use css variables for colors.
| height: 105px; | ||
| background: #fdeeed; | ||
| border: 1px solid #f9cdc8; | ||
| border-radius: 8px; |
There was a problem hiding this comment.
please use css variables for this. --border-radius-l
| .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; | ||
| } |
There was a problem hiding this comment.
look at the comments in the similar CSS above (table-fetch-error class)
| $: isTablesLoading = $tablesStore.state === States.Loading; | ||
| $: isTablesUnfetchable = $tablesStore.state === States.Error; | ||
| $: isExplorationsLoading = $queries.requestStatus.state === 'processing'; | ||
| $: isExplorationsUnfetchable = $queries.requestStatus.state === 'failure'; |
There was a problem hiding this comment.
instead of passing separate variables for loading & error, please use RequestStatus (refer: mathesar_ui/src/api/utils/requestUtils.ts). This will let you pass error message too to the child component.
You can then use the error string to show on the UI instead a generic message("Error fetching tables") from Figma. This give more information to the user and help him/her to report better errors .
rajatvijay
added
pr-status: revision
|
Thanks, @rajatvijay. However, my scary mid-sem is going on. It will take one week and a half for me to get back to this. |
|
@Aritra8438 are you still planning to finish this PR? |
|
Yes, @seancolsen. |
|
I'm closing this because it's not been updated in a while, and the issue now seems to have been solved by #3323. |
This PR is related to #2644.
Technical details:
Currently, when one of the APIs
fails, the schema page shows no errors, assuming no results with success state. This PR will,
Screencast:
Schema_fetch_failure.mp4
Design Reference:
Figma link: https://www.figma.com/file/xHb5oIqye3fnXtb2heRH34/Styling?type=design&node-id=5807%3A31921&t=O8X0oEtOLpfjubpU-1
Checklist
Update index.md).developbranch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin