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

feat(app): Show connect alert banner on successful connection #1700

Merged
merged 2 commits into from
Jun 15, 2018

Conversation

Kadee80
Copy link
Contributor

@Kadee80 Kadee80 commented Jun 14, 2018

overview

This PR closes #1314. Will open a separate issue for connectRequest error banner.

screen shot 2018-06-14 at 2 19 32 pm

changelog

  • feat(app): Show connect alert banner on successful connection

review requests

  • connection banner displays on successful robot connect
  • connection banner is dismissible
  • dismissed banner reappears on disconnect and reconnect

@Kadee80 Kadee80 added feature Ticket is a feature request / PR introduces a feature app Affects the `app` project ready for review labels Jun 14, 2018
@Kadee80 Kadee80 requested review from mcous, IanLondon and b-cooper June 14, 2018 18:53
Copy link
Contributor

@mcous mcous left a comment

Choose a reason for hiding this comment

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

📲

@@ -58,6 +59,7 @@ function RobotSettingsPage (props: Props) {
return (
<Page>
<TitleBar title={robot.name} />
<ConnectBanner {...robot} key={Number(robot.isConnected)}/>
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of Number here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Flow was expected number or string not a bool

const TITLE = `${name} successfully connected`
return (
<div>
{isVisible && (
Copy link
Contributor

Choose a reason for hiding this comment

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

Could also do an early null return if !isVisible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i like that. will change in a bit

@codecov
Copy link

codecov bot commented Jun 14, 2018

Codecov Report

Merging #1700 into edge will decrease coverage by 0.11%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #1700      +/-   ##
==========================================
- Coverage   34.68%   34.57%   -0.12%     
==========================================
  Files         342      343       +1     
  Lines        5576     5594      +18     
==========================================
  Hits         1934     1934              
- Misses       3642     3660      +18
Impacted Files Coverage Δ
app/src/pages/Robots.js 0% <0%> (ø) ⬆️
app/src/components/RobotSettings/ConnectBanner.js 0% <0%> (ø)
protocol-designer/src/components/App.js 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aeb1785...1575398. Read the comment docs.

@Kadee80 Kadee80 merged commit 70cd8b2 into edge Jun 15, 2018
@Kadee80 Kadee80 deleted the app_connect-success-banner branch June 15, 2018 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Affects the `app` project feature Ticket is a feature request / PR introduces a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Usability: Provide clear indication of successful connection
2 participants