Skip to content
This repository has been archived by the owner on Apr 15, 2019. It is now read-only.

Fix offline behaviour - Closes #545 #612

Merged
merged 9 commits into from
Aug 21, 2017
Merged

Conversation

slaweet
Copy link
Contributor

@slaweet slaweet commented Aug 17, 2017

Closes #545

@slaweet slaweet self-assigned this Aug 17, 2017
Copy link
Contributor

@reyraa reyraa left a comment

Choose a reason for hiding this comment

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

Prefer an overlaying layer (Fixed positioned over all elements) to hinder all user interaction and not apply all these changes.

@slaweet
Copy link
Contributor Author

slaweet commented Aug 18, 2017

The thing is we don't want to disable all interactions. Definitely not "Logout" button

@slaweet
Copy link
Contributor Author

slaweet commented Aug 18, 2017

Now that I'm thinking about it "SIgn/verify message" has no problem with being offline, so it should be available

@slaweet
Copy link
Contributor Author

slaweet commented Aug 18, 2017

But for the migration, I think the current implementation is fine and there can be an "enhancement" issue later on to "Provide a better offline experience".

@reyraa
Copy link
Contributor

reyraa commented Aug 18, 2017

Ok, let me describe in more details.
We have 2 kind of interactions, the ones required an online/active peer, the ones that don't need.
We have 2 kind event after interaction, either we navigate or open a modal.

These said, we can simply add a class name to buttons/menu items which require the active peer to disable them by CSS later on when the body tag, has a class name of is-offline for example.
In a more informative UX approach, we can have a middleware that prevents opening the requested modal or navigation request and shows an alert meaning this action in only available if there is an active peer available.

@slaweet
Copy link
Contributor Author

slaweet commented Aug 18, 2017

I'm all for "a more informative UX approach".
I can think of tooltips saying something like "Not available while offline". My OfflineWrapper component could easily do that in the future.

But the goal of this PR is merely migrating the functionality from Angular.
Using a wrapper component is IMO more readable and more React then using two classes we had in Angular.

@reyraa
Copy link
Contributor

reyraa commented Aug 18, 2017

The reason I don't approve your implementation is:

  • The way you apply the wrapper is inconsistent. In a case, you've wrapped a whole HOC with routings in it. in other case, a whole mane, and in the end a button.
  • All wrapping and adding an extra layer of complication just to wrap them in a span with a class name? why not simply a class name?

Please change the offlineWrapper to just manipulate a class name and not add any layers to the existing structure.

thanks.

Copy link
Contributor

@reyraa reyraa left a comment

Choose a reason for hiding this comment

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

Thank you Vit. Good job!

@slaweet slaweet merged commit c4706b8 into development Aug 21, 2017
@slaweet slaweet deleted the 545-fix-offline-behaviour branch August 21, 2017 15:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants