Skip to content
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

ui: Add error boundary in the React UI #2759

Merged
merged 4 commits into from
Jun 17, 2020
Merged

Conversation

onprem
Copy link
Member

@onprem onprem commented Jun 14, 2020

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

  • Add a error boundary to the React UI. Now in case of an irrecoverable error, users would see a "Something went wrong" page instead of a blank screen.
  • Fix the crash when there is no store connected to the Querier. The crash was because the API response for /api/v1/label/__name__/values was
{
  "status": "success",
  "data": null
}

Here, the UI expects data to be an array.
Fixed this by sending an empty array ([]) instead of null

Verification

Tested locally by emulating error conditions.

Screenshot

Error Boundary

Signed-off-by: Prem Kumar <prmsrswt@gmail.com>
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Awesome! One comment, then LGTM

pkg/strutil/merge.go Outdated Show resolved Hide resolved
Signed-off-by: Prem Kumar <prmsrswt@gmail.com>
Copy link
Member

@squat squat left a comment

Choose a reason for hiding this comment

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

Looks great :)))

Thanos issue tracker.
</a>
</h3>
<Button color="link" id="toggler" className={styles.detailsBtn} style={{ fontSize: '1.2em' }}>
Copy link
Member

Choose a reason for hiding this comment

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

If this is the only instance of the detailsBtn class, why split the font size out of the css? To keep the css class more generic?

Copy link
Member Author

Choose a reason for hiding this comment

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

The internal bootstrap classes were getting priority over the detailsBtn class, so I wasn't able to modify the font-size here. I had two options, either use !important on use inline CSS.

I think maybe there is a third solution too. I can increase the specificity of detailsBtn class, by declaring it as button.detailsBtn or something.

Copy link
Member Author

Choose a reason for hiding this comment

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

Using button.detailsBtn instead of just .detailsBtn worked 🎉

Copy link
Member

Choose a reason for hiding this comment

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

yes let's avoid !important for sure. making it more specialized 👍

Signed-off-by: Prem Kumar <prmsrswt@gmail.com>
@squat
Copy link
Member

squat commented Jun 17, 2020

looks great. See,s that we need to fix a linting error. This may require regeneration of the bindata.go

Signed-off-by: Prem Kumar <prmsrswt@gmail.com>
@squat squat merged commit 8439d36 into thanos-io:master Jun 17, 2020
@onprem onprem deleted the error-boundary branch June 17, 2020 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants