remove error alert when no internet and add error description in the popover #309

rvermeiren · 2018-01-22T16:13:20Z

Remove error alert when no internet and add error description in the popover

Please be sure to read the contributor's guide before submitting any pull requests.

Requirements

Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
All new code requires tests to ensure against regressions.

Description of the Change

To avoid to generate an error when the internet is not connected, delete the error throw when initialization fails and move the message in the popover launch when the internet is not connected.

I also try to change the error thrown when the connection is lost since the initialization but don't have understood yet how it's work.

Alternate Designs

As suggested in the issue (#238), remove the error thrown when initialization of teletype. And pass the message to the popover. It's suggested because not having internet is not really an error.

Benefits

An error less

Possible Drawbacks

Maybe another error can catch and pass to the popover.

Verification Process

Run atom without internet and with teletype installed. --> Tooltip contains the error and suggest to restart.

Run atom with internet --> correct behaviour.

I can't run the local test on my computer as it seems to be an error with the postgresql database.

Applicable Issues

Issue (#238) :
Show error message in tooltip when initialization fails

jasonrudolph

@rvermeiren: Thanks for this contribution. 🙇 Below are a few changes that will be needed in order to get this pull request merged. I hope this helps.

jasonrudolph · 2018-01-22T18:04:41Z

lib/package-initialization-error-component.js

@@ -16,6 +16,7 @@ class PackageInitializationErrorComponent {
  render () {
    return $.div({className: 'PackageInitializationErrorComponent'},
      $.h3(null, 'Teletype initialization failed'),
+      $.p(null, 'Error : ' + this.props.initializationError.message ),


The space before the closing parenthesis is causing the linter to fail the build: https://travis-ci.org/atom/teletype/jobs/331883465#L550

@rvermeiren: Can you please remove that space? You can run the linter locally with npm run lint to verify that the issue is resolved.

Once you resolve that linter failure, the CI build will run the tests, and you'll see that this test fails. Can you update that test to reflect the changes implemented in this pull request?

jasonrudolph · 2018-01-22T18:06:05Z

lib/teletype-package.js

+        // this.notificationManager.addError('Failed to initialize the teletype package', {
+        //   description: `Establishing a teletype connection failed with error: <code>${error.message}</code>`,
+        //   dismissable: true
+        // })


We prefer to delete code that we're no longer using. Can you please delete this commented-out code and the else block that contains it?

jasonrudolph · 2018-01-22T18:20:15Z

I captured a before-and-after screenshot in case it's helpful for anyone else reviewing this pull request:

Before

Error notification and error shown in popover.

After

No error notification. Instead, the error message appears in the popover.

rvermeiren · 2018-01-22T19:58:04Z

Hi @jasonrudolph , first, thank you for your attention. This is my first contribution on Github and I'm very thankful for your help and review.
I have fix the lint issue and the comment code you find.

For the testing, I remove two test because the scenario was wrong. When internet is turned of, initializationError not allow to share portal or join one. So, these scenario were good before because the test catch the old exception in place of the good one (tested in another file). So i just update the good test and remove the two other.

I hope this is ok and my contribution is good. Regards, Rémy

jasonrudolph

So i just update the good test and remove the two other.

👍

I hope this is ok and my contribution is good.

Thanks for contributing! 🍻

Squashed commit of the following: commit 30b14a4 Author: Jason Rudolph <[email protected]> Date: Mon Jan 22 17:43:24 2018 -0500 Tweak error message formatting commit 5108745 Author: Vermeiren Rémy <[email protected]> Date: Mon Jan 22 20:45:47 2018 +0100 remove incorrect scenario commit e114a5d Author: Vermeiren Rémy <[email protected]> Date: Mon Jan 22 20:37:33 2018 +0100 update test commit fef7fde Author: Vermeiren Rémy <[email protected]> Date: Mon Jan 22 20:32:32 2018 +0100 trying to make test more clear and generic commit 31e2c46 Author: Vermeiren Rémy <[email protected]> Date: Mon Jan 22 20:24:07 2018 +0100 fix test issues with isEquals commit df6b6e7 Author: Vermeiren Rémy <[email protected]> Date: Mon Jan 22 20:17:57 2018 +0100 fix error in test commit 87b2811 Author: Vermeiren Rémy <[email protected]> Date: Mon Jan 22 20:05:47 2018 +0100 remove some fail test because i think it's unconsistent commit 0f0ac85 Author: Vermeiren Rémy <[email protected]> Date: Mon Jan 22 19:58:36 2018 +0100 fix pull request issue with lint, delete not used code and edit test to fit the new behaviour commit f6f24c6 Author: Vermeiren Rémy <[email protected]> Date: Mon Jan 22 15:16:28 2018 +0100 remove error alert when no internet and add error description in the popover

rvermeiren · 2018-01-22T23:14:32Z

Thank you for helping me with my first contribution. I think it was a great experience and I will probably continue to contribute further.

I see that one check not passed with the merge. Can I do something about that ?

winstliu · 2018-01-22T23:53:24Z

Nice work @rvermeiren! We would love to see you contribute more :).

You do not have to worry about the pending status check.

jasonrudolph suggested changes Jan 22, 2018

View reviewed changes

jasonrudolph approved these changes Jan 22, 2018

View reviewed changes

jasonrudolph merged commit 30b14a4 into atom:master Jan 22, 2018

jasonrudolph force-pushed the master branch from da24717 to 30b14a4 Compare January 22, 2018 23:01

jasonrudolph mentioned this pull request Jan 22, 2018

Show error message in tooltip when initialization fails #238

Closed

1 task

jasonrudolph mentioned this pull request Jan 31, 2018

Error occurs when there's no internet #313

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove error alert when no internet and add error description in the popover #309

remove error alert when no internet and add error description in the popover #309

rvermeiren commented Jan 22, 2018

jasonrudolph left a comment

jasonrudolph Jan 22, 2018

jasonrudolph Jan 22, 2018

jasonrudolph Jan 22, 2018

jasonrudolph commented Jan 22, 2018

rvermeiren commented Jan 22, 2018 •

edited

Loading

jasonrudolph left a comment

rvermeiren commented Jan 22, 2018

winstliu commented Jan 22, 2018

remove error alert when no internet and add error description in the popover #309

remove error alert when no internet and add error description in the popover #309

Conversation

rvermeiren commented Jan 22, 2018

Requirements

Description of the Change

Alternate Designs

Benefits

Possible Drawbacks

Verification Process

Applicable Issues

jasonrudolph left a comment

Choose a reason for hiding this comment

jasonrudolph Jan 22, 2018

Choose a reason for hiding this comment

jasonrudolph Jan 22, 2018

Choose a reason for hiding this comment

jasonrudolph Jan 22, 2018

Choose a reason for hiding this comment

jasonrudolph commented Jan 22, 2018

Before

After

rvermeiren commented Jan 22, 2018 • edited Loading

jasonrudolph left a comment

Choose a reason for hiding this comment

rvermeiren commented Jan 22, 2018

winstliu commented Jan 22, 2018

rvermeiren commented Jan 22, 2018 •

edited

Loading