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: Messaging continuity when node connection lost #1577

Merged
merged 24 commits into from
Sep 1, 2020

Conversation

jessicaschilling
Copy link
Contributor

About

This PR addresses inconsistency of experience when the user loses API connection but still has Web UI open. See #1575 for original issue details.

Done

  • On Welcome page, "in this app" and "what is IPFS" boxes only appear if the user is connected
  • On Status page, if connection is lost, displays the same "Could not connect to the IPFS API" troubleshooting box as on the Welcome page
  • Assorted cleanup required to facilitate this (note: would appreciate a close code review on this, thanks in advance!)
    • AboutWebUI and AboutIpfs get their own components
    • IsConnected gets its own component
    • IsNotConnected gets its own component (which is used on the Status page now too)
    • Removed old Status page component StatusNotConnected and its associated i18n/storybook stuff

To do

Please note that these will require some help from @lidel and/or @rafaelramalho19 ...

  • Improve detection/reload logic so user doesn't get "stuck" on a page with outdated information, not knowing they've dropped API connection
    • This could mean creating a separate "you're not connected any more" page that includes the IsNotConnected component, or it could just mean displaying the IsNotConnected component at the top of each individual page if connection is lost (this is how it's currently done on Status page)

Closes #1575.

@jessicaschilling
Copy link
Contributor Author

@lidel -- per our chat earlier today, I believe this addresses everything except improving the detection/reload logic for when connection is dropped. I did move quite a bit around from WelcomePage.js into IsNotConnected.js, so would appreciate a close review, but everything appears to be working as expected. Let me know if I can help with anything else. Thanks!

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

LGTM!

ps. found some new nits, but those are not related to this PR:

  • when I click on the 2020-08-10--18-04-53 "Advanced" section gets expanded. But if I manually collapse this section and click on 2020-08-10--18-04-53 again it no longer expands.
  • clicking on "Edit" opens Settings, but does not scroll to "API Address" section

@jessicaschilling
Copy link
Contributor Author

Regarding nit 1, @rafaelramalho19, do you have any suggestions, since you added this bit in originally? 😊

Regarding nit 2, looks like anchor tags don't work as expected with react-router: https://stackoverflow.com/questions/48223566/using-anchor-tags-in-react-router-4
We could manually specify scroll position, or install react-router-hash-link, but neither of those feel particularly pretty. :(

Regarding detection/reload logic -- is that something you'd want to do in this PR, or break it out into separate work?

@rafaelramalho19
Copy link
Contributor

rafaelramalho19 commented Aug 10, 2020

@jessicaschilling related to nit1, here's the fix: #1579

Nit 2 should be addressed with react-router-hash-link 😄

@lidel lidel added this to the v2.11 milestone Aug 14, 2020
@jessicaschilling
Copy link
Contributor Author

@lidel Unfortunately it looks like using react-router-hash-link isn't going to work unless we pull the default use of # in all our URLs. Is direct-linking critical at this point? I might recommend we punt on this until later.

@jessicaschilling
Copy link
Contributor Author

jessicaschilling commented Aug 14, 2020

After discussion with @lidel, rearranging Settings page slightly as a workaround:
image

What changed:

  • Swapped API Address and Language boxes
  • brought analytics disclaimer “No CIDs, filenames …” full width of page and tightened margin between that and “Configure what is collected”
  • Removed the superfluous “API ADDRESS” label above the box for changing API - put that i18n item inside the input itself as aria-labelledby={t('apiAddressForm.apiLabel')} for accessibility -- @rafaelramalho19, can you please confirm that this is the right way to handle this a11y-wise?

Re-requesting reviews all around. Thanks!

@jessicaschilling jessicaschilling changed the title [WIP] feat: Messaging continuity when node connection lost feat: Messaging continuity when node connection lost Aug 14, 2020
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Two new(?) nits:

  1. While on Settings screen, when I change API address to invalid one, or when my node goes offline, the last revision of config is still displayed. It may be confusing, as user may glance at invalid config from the old node and not realized webui failed to connect to the new one yet. Would it be ok to hide the JSON editor when API is offline?

    2020-08-17--23-54-48

  2. When I shut down my node and open Files screen (without reloading the page), it displays connection error AND import drawer is expanded with "Imported 0 items" message – it hit me how confusing it looks. Quick fix would be to redirect to status page when offline, or at least hide the import drawer, if possible. If we only had Orange error, then user would instinctively refresh the page:

    2020-08-17--23-56-23

@rafaelramalho19 since you are the most familiar with the import drawer, is there a quick fix for (2)? Also, mind taking a look at the a11y question from previous comment?

@jessicaschilling
Copy link
Contributor Author

jessicaschilling commented Aug 17, 2020

Would it be ok to hide the JSON editor when API is offline?

Agreed! @rafaelramalho19, do you mind including this in the changes @lidel mentions above? Thank you 😊

@jessicaschilling
Copy link
Contributor Author

jessicaschilling commented Aug 26, 2020

Resolved the conflict created by CLI tutor mode merge. Also, CircleCI is behaving again 😊

I think we have two remaining items here:

  1. @rafaelramalho19, what are your thoughts on @lidel's question 2 in feat: Messaging continuity when node connection lost #1577 (review) ?
    2. @rafaelramalho19, did I use aria-labelledby correctly in the final bullet in feat: Messaging continuity when node connection lost #1577 (comment) ?

Thank you!

@jessicaschilling
Copy link
Contributor Author

@rafaelramalho19 Thanks for the a11y clarity 😊
Do you mind looking at the other question from Lidel? Re-pasted below since it gets a little lost in the thread. Thanks!

When I shut down my node and open Files screen (without reloading the page), it displays connection error AND import drawer is expanded with "Imported 0 items" message – it hit me how confusing it looks. Quick fix would be to redirect to status page when offline, or at least hide the import drawer, if possible. If we only had Orange error, then user would instinctively refresh the page:

2020-08-17--23-56-23

@rafaelramalho19
Copy link
Contributor

I wasn't able to replicate because I got redirected to the welcome page, is this replicable? 🤔

@jessicaschilling
Copy link
Contributor Author

@rafaelramalho19 Afraid I was just able to replicate it on my local :(

@lidel
Copy link
Member

lidel commented Aug 28, 2020

@rafaelramalho19 to replicate, avoid refreshing the page. Try:

  1. be on the Status or Peer screen and when node is online and everything works ok
  2. shut down your node, so webui loses connection to the API
  3. switch to Files screen (without refreshing the page)

@lidel
Copy link
Member

lidel commented Sep 1, 2020

Perhaps we should unblock v2.11 and merge this as-is and fill new issue for connection error AND expanded import drawer?
Most of the users will never end up on that screen with that state.

@jessicaschilling
Copy link
Contributor Author

@lidel This makes good sense, will do.

@jessicaschilling
Copy link
Contributor Author

Files page visual continuity issue is in #1606. Per #1577 (comment), merging.

@jessicaschilling jessicaschilling merged commit 66df07a into master Sep 1, 2020
@jessicaschilling jessicaschilling deleted the feat/nodestop-continuity-ui branch September 1, 2020 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve messaging/continuity when node shut down but UI still open
3 participants