Skip to content
This repository has been archived by the owner on Jun 17, 2021. It is now read-only.

Disable actions that require an internet connection #352

Closed
5 tasks done
AWolf81 opened this issue Jan 20, 2019 · 8 comments
Closed
5 tasks done

Disable actions that require an internet connection #352

AWolf81 opened this issue Jan 20, 2019 · 8 comments
Labels
bug Something isn't working Discussion Something to discuss good first issue Good for newcomers

Comments

@AWolf81
Copy link
Collaborator

AWolf81 commented Jan 20, 2019

It's more a bug than a feature but I'm writing this as feature description. I've noticed this during my work on PR #335 because there I'm using fetch to get the starters from Github.

Describe the bug
With-out internet connection some features aren't working as expected:

  • Create new project will hang and show a console.log (see first screenshot)
  • Add dependency won't show Agolia results with-out an info what's wrong
  • Gatsby starters (once merged) not visible in Typeahead list

To Reproduce

  1. Disable Wifi or disconnect LAN cable
  2. Trigger task that requires a connection:
  • Create a new project & click Let's build
  • Click Add New Dependency
  • Note: Gatsby starter list (new feature in gatsby-starter branch) won't need a special handling as failing silently is OK.
  1. Check console.

Expected behavior
Allow tasks that can be performed offline & disable actions that require an internet connection. Also display an infobar that Guppy requires an internet connection on the top of the app (right below the application menu).

  • OnlineCheck Component: Use navigator.onLine to determine & check if this is working in Chrome. See mdn here. Added to App.js. Check can be done like in the following Codesandbox example - just Redux missing.
  • Add isOnline boolean to app-status.reducer
  • Check internet connection before tasks that require a connection (on failure show infobar & disable action - depends on how the online check is working, maybe automatically disabled & just a connection to app-status.reducer - isOnline flag required)
    • Create new project build
    • Before opening Add New Dependency

Screenshots
CRA build with-out internet
grafik

Add dependency fails silently
grafik

Discussion

  • Do we need offline support? I think it would be good but until we have it, the easiest fix is to disable these action. Offline support for project creation would require a cache for project creation. Adding dependencies is probably not possible to cache - too much data. Note: This requires a new issue so we can discuss the details.
  • UI: Is adding an infobar on top of the screen OK? Should it be dismissable?
@AWolf81 AWolf81 added bug Something isn't working good first issue Good for newcomers Discussion Something to discuss labels Jan 20, 2019
@AWolf81 AWolf81 changed the title Disable actions that require an internet connection & connection tests Disable actions that require an internet connection Jan 20, 2019
@actuallyReallyAlex
Copy link

I would like to add that it seems if your network is heavily throttled, the search feature also doesn't work, as described above. Creating a new project shows the animation and takes a long time, but it does eventually work.

Hard for me to tell if this is a proxy issue, a firewall issue, or a slow connection issue.

@joshuaellis
Copy link
Contributor

If no one is looking at this I'm happy to do so.

@AWolf81
Copy link
Collaborator Author

AWolf81 commented Feb 4, 2019

@joshuaellis sure, it's all yours.
If you're having questions or need support during work - please let me know.

I'm pretty busy with writing unit tests. That's why I'm not working on new features at the moment. But maybe I'm doing a break from the tests soon and have a look at the Codesandbox export feature.

@joshuaellis
Copy link
Contributor

UI: Is adding an infobar on top of the screen OK? Should it be dismissable?

A couple of questions relating to the quote, firstly do we already have an infobar designed and built? Not to worry if not. Secondly, I don't think it should be dismissable considering we're going to be blocking core features of the product – building new projects and adding new dependencies.

What're your thoughts?

@AWolf81
Copy link
Collaborator Author

AWolf81 commented Feb 5, 2019

We don't have an infobar designed. That's a new part of the UI.
Yes, you're right. It's OK that it is not dismissable.

@joshuaellis
Copy link
Contributor

I was wondering if we want to have the infobar UI as a separate component that we could use in other instances, is this something we would want or should the Online Checker render an infobar?

@AWolf81
Copy link
Collaborator Author

AWolf81 commented Feb 20, 2019

I think it's OK to render it in in the OnlineCheck component as this is a special info.

We have had some discussions about notifications (nothing decided) but I think this won't fit into a toastr notification system. We wanted to have some toasts for finishing tasks info or when we'd like to have the users attention on a task - I think there is no open issue for this but I'll check and open an issue so we can discuss this there.

AWolf81 pushed a commit that referenced this issue Mar 19, 2019
* Online check component added

* add reducer & action for online check component.

also included in App.js

* removed sidebar functionality when offline

* removed depedency functionality when offline

* removed functionality from create new project wizard when offline

* fixed tests to align with disabled pattern

* add infoBanner to Z-indexes for future usage.

* removed random {' '}, changed IS_ONLINE_CHECK to SET_ONLINE_STATUS

* pull request requested changes

missed one file.

* updated infobar to be fixed header.

* flow fix

* remove random whitespace

* Pull request changes

remove state type, move styled components to before redux part, change background color of infobar to be transparent.

* enable menu items based on isOnline
@AWolf81
Copy link
Collaborator Author

AWolf81 commented Mar 19, 2019

OK, we can close this. Thanks @joshuaellis for adding this.

I like the idea of having a low bandwidth info as @alexlee-dev mentioned. Maybe we could add this as a new issue but I'm not sure if it's worth to add.
If you like to see this feature please create a new issue and we can discuss the implementation details there.

@AWolf81 AWolf81 closed this as completed Mar 19, 2019
Haroenv pushed a commit that referenced this issue Mar 27, 2019
* Online check component added

* add reducer & action for online check component.

also included in App.js

* removed sidebar functionality when offline

* removed depedency functionality when offline

* removed functionality from create new project wizard when offline

* fixed tests to align with disabled pattern

* add infoBanner to Z-indexes for future usage.

* removed random {' '}, changed IS_ONLINE_CHECK to SET_ONLINE_STATUS

* pull request requested changes

missed one file.

* updated infobar to be fixed header.

* flow fix

* remove random whitespace

* Pull request changes

remove state type, move styled components to before redux part, change background color of infobar to be transparent.

* enable menu items based on isOnline
Haroenv added a commit that referenced this issue Mar 27, 2019
* add analytics tags

* Update AddDependencySearchProvider.js

* #352 Disable actions that require an internet connection (#368)

* Online check component added

* add reducer & action for online check component.

also included in App.js

* removed sidebar functionality when offline

* removed depedency functionality when offline

* removed functionality from create new project wizard when offline

* fixed tests to align with disabled pattern

* add infoBanner to Z-indexes for future usage.

* removed random {' '}, changed IS_ONLINE_CHECK to SET_ONLINE_STATUS

* pull request requested changes

missed one file.

* updated infobar to be fixed header.

* flow fix

* remove random whitespace

* Pull request changes

remove state type, move styled components to before redux part, change background color of infobar to be transparent.

* enable menu items based on isOnline

* Setup ESlint rules for Jest (#366)

* WIP: Added eslint-jest & fixed issues (except snapshots)

* reduced snapshot size to meet max 100 lines

* fixed snapshot

* changed comment to a note about noPadding prop

* updated snapshot

* Test ProjectConfigurationModal component (#360)

* WIP: Added render test

* WIP: Added tests

* fixed flow

* added focus test

* Merge branch 'master' into test-project-configuration-modal

* fixed linting warnings

* removed typing as no flow used in test files

* Test CreateNewProjectWizard components (#361)

* WIP: Added tests

* WIP: ProjectType undefined not throwing in test

* fixed flow

* added tests

* reverted to children render props

* Merge branch 'master' into test-create-new-project-wiz

* Fixed linting & replaced snapshot with simple smoke tests

* addressed review comments

* Test DependencyManagementPane component (#363)

* moved related components to DependencyManagementPane

* added tests

* fixed flow

* added a constant instead of Date.now()

* updated snapshot

* removed white-spaces

* removed verbose

* fixed linting & mocked AlgoliaLogo

* move related component files & moved tests to __tests__ folder

* Add script for test:dev

* Fix spaces and update lint-staged (#369)

* Switch to advanced config to silence validation warning

* Fix spacing issues

* chore: fix merge changes
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working Discussion Something to discuss good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants