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

refactor(app): display alert-circle in robot tab if any calibration is marked bad #6948

Merged
merged 4 commits into from
Nov 9, 2020

Conversation

ahiuchingau
Copy link
Contributor

Overview

closes #6604
This PR adds a warning icon on the robot tab if any calibration (deck cal, pip offset, tip length) is marked bad. See design

Changelog

  • add optional warningReason prop to NavBarLink to display the alert icon
  • update tests to reflect new changes

Review requests

This is what the new robot tab now looks like:
Screen Shot 2020-11-09 at 3 26 40 PM

If there's an update available for the robot, the notification bubble will be shown instead of the warning icon. Is that ok? Or should we show both at the same time?

Risk assessment

low

@ahiuchingau ahiuchingau added app Affects the `app` project refactor labels Nov 9, 2020
@ahiuchingau ahiuchingau requested a review from a team November 9, 2020 20:46
@ahiuchingau ahiuchingau requested a review from a team as a code owner November 9, 2020 20:46
@ahiuchingau ahiuchingau requested review from Laura-Danielle and Kadee80 and removed request for a team November 9, 2020 20:46
@ahiuchingau ahiuchingau changed the title refactor(app): display alert-circle in robot tab if any cal data is marked bad refactor(app): display alert-circle in robot tab if any calibration is marked bad Nov 9, 2020
@emilywools
Copy link

hey @ahiuchingau! how about this:

  • if there's an update notification, you see it
  • if there's a warning, you get the orange !
  • if there's both an update and a warning you just get the orange !

so only show 1 bubble max

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Code looks good but

  • Let's switch the order of precedence (per @emilywools 's comment above) so if there's a warning and a notification, you get the warning icon and tooltip
  • Let's add a test for the order of precedence

@ahiuchingau ahiuchingau requested a review from sfoster1 November 9, 2020 22:24
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

LGTM!

@ahiuchingau ahiuchingau merged commit 4075aa5 into chore_release-4.0.0-beta.0 Nov 9, 2020
@ahiuchingau ahiuchingau deleted the app_warn-if-deck-cal-is-bad branch November 9, 2020 22:36
@nusrat813 nusrat813 self-requested a review November 9, 2020 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Affects the `app` project refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants