Skip to content

Use interstitial page instead of infobar for ipfs #7918

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

Merged
merged 16 commits into from
Feb 19, 2021
Merged

Conversation

spylogsster
Copy link
Contributor

@spylogsster spylogsster commented Feb 10, 2021

Resolves brave/brave-browser#13655

  • Implemented interstitial page for IPFS ASK mode in settings
  • Show IPFS interstitial instead of IPFS Infobar
  • Show node installation progress and errors on page
  • Removed IPFS Infobar

Submitter Checklist:

  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)
  • Requested a security/privacy review as needed

Reviewer Checklist:

  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

  • Set IPFS mode in settings as ASK
  • Navigate any ipfs url, ipfs://bafkreigcnxudvpojjfwncmauociy5q46zsq46oe66cxbyzie3imabuoege for example
  • You should see the interstitial page with source url in address bar
  • In case of error during IPFS node installation(or launching), an error message should be visible
  • Change browser theme in settings, the page should apply new theme without reloading

@spylogsster spylogsster self-assigned this Feb 10, 2021
@spylogsster spylogsster requested a review from bbondy February 10, 2021 16:44
@spylogsster spylogsster requested a review from a team as a code owner February 10, 2021 18:24
@spylogsster spylogsster force-pushed the feature-13655 branch 12 times, most recently from 4aafb22 to abdfb09 Compare February 15, 2021 13:57
@spylogsster spylogsster force-pushed the feature-13655 branch 4 times, most recently from 0aa2343 to 96e0bba Compare February 15, 2021 19:16
Copy link
Member

@bbondy bbondy left a comment

Choose a reason for hiding this comment

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

Currently when you install for the first time, your first experience will be this:
Screen Shot 2021-02-16 at 11 52 25 AM

Let's instead have another button state like the Installing... one called Waiting for peers.... In which you will wait up to 2 minutes for the connected peers to go above 0. As soon as it does, you will only then redirect the user to the local node. Maybe check every 1s.
Screen Shot 2021-02-16 at 11 52 21 AM

@spylogsster spylogsster force-pushed the feature-13655 branch 5 times, most recently from e02a6fc to 65eeb74 Compare February 17, 2021 14:09
@spylogsster
Copy link
Contributor Author

spylogsster commented Feb 17, 2021

Currently when you install for the first time, your first experience will be this:
Screen Shot 2021-02-16 at 11 52 25 AM

Let's instead have another button state like the Installing... one called Waiting for peers.... In which you will wait up to 2 minutes for the connected peers to go above 0. As soon as it does, you will only then redirect the user to the local node. Maybe check every 1s.
Screen Shot 2021-02-16 at 11 52 21 AM

image
image

@bbondy
Copy link
Member

bbondy commented Feb 17, 2021

When we add this error text we should keep the padding and not push the button up against it's box.
Screen Shot 2021-02-17 at 12 57 31 PM

For example see:
Screen Shot 2021-02-17 at 12 58 17 PM
Notice that it retains the padding both at the bottom and the top.

Also I think we can avoid showing any red text error at all until after 10s. Because it is kind of seen like an error when we display it.

@bbondy
Copy link
Member

bbondy commented Feb 17, 2021

I tried disconnecting my ethernet cable too when it is installing, it seems to just hang here and I think it never gives an error state.

Screen Shot 2021-02-17 at 1 02 41 PM

@spylogsster
Copy link
Contributor Author

hang here and I think it never gives an error state.

after short discussion we decided to proceed with it in separate issue brave/brave-browser#14229

@spylogsster spylogsster force-pushed the feature-13655 branch 3 times, most recently from 9bd0ab6 to e187e13 Compare February 18, 2021 11:31
@bbondy bbondy merged commit dc64a97 into master Feb 19, 2021
@bbondy bbondy deleted the feature-13655 branch February 19, 2021 04:42
@spylogsster spylogsster added this to the 1.22.x - Nightly milestone Feb 19, 2021
@karenkliu
Copy link

@spylogsster Yes what @bbondy said ☝️ Please keep the padding when there is text below the button. If it's not an error you can use the same font size/styling as the error text but make it the regular dark text color instead of the red error color.

Copy link

@kloun kloun left a comment

Choose a reason for hiding this comment

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

ььь

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.

Use full page explainer for unconfigured ipfs:// loads instead of loading content with an infobar
5 participants