Skip to content

Feature handle api status #220

Merged
merged 8 commits into from
Jul 14, 2021
Merged

Feature handle api status #220

merged 8 commits into from
Jul 14, 2021

Conversation

wrigh393
Copy link
Collaborator

Closes #168

@wrigh393 wrigh393 requested a review from campb303 July 12, 2021 22:15
// Create stateful variables.
const [sidebarOpen, setSidebarOpen] = useState(false);
const [queues, setQueues] = useState([]);
const [items, setItems] = useState([]);
const [selectedQueues, setSelectedQueues] = useState([]);
const [queueSelectorOpen, setQueueSelectorOpen] = useState(false);
const [isLoading, setIsLoading] = useState(false);
const [error, setError] = useState(false)
const [response, setResponse] = useState({ status: '', message: '' })
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generic variable name. Replace with errorMessage and set to an initial value of empty string. Populate the message later if need be.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, do you mean to replace both of these lines with one variable?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After sleeping on it, methinks line 22 can be nixed.

Comment on lines 63 to 69
if (apiResponse.ok) {
setQueues(queueJson);
setIsLoading(false)
} else {
setResponse({ status: apiResponse.status, message: apiResponse.statusText })
setError(true)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code checks for errors after a bad request is received. This should run before line 61. It should also be replaced with a guard statement fo if (!apiResponse.ok) {...} that handles errors and returns. Code to follow should be what happens without error and not inside the else condition.

Comment on lines 131 to 141
/>{error ? <Box classes={{ root: classes.errorContainer }}>
<Typography variant="h1">
{response.status}
</Typography>
<Typography variant="h3">
Something went wrong.
</Typography>
<Typography variant="h5">
{response.message}
</Typography>
</Box> : <ItemTable data={items} rowCanBeSelected={sidebarOpen} loading={isLoading} />}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The information hierarchy emphasises the wrong information. On error, the API will send a JSON object with a message key and value describing the error. Replace this text with an appropriate icon and text below with the format of response.message + "\n" + "Error Code: " + response.status similar to: error message

@campb303 campb303 merged commit fd194f5 into staging Jul 14, 2021
@campb303 campb303 deleted the feature-handle-api-status branch July 14, 2021 17:42
Sign in to join this conversation on GitHub.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants