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

Routes for going directly to Become a Provider #193

Merged
merged 8 commits into from
Nov 11, 2024

Conversation

shannonwells
Copy link
Collaborator

@shannonwells shannonwells commented Nov 7, 2024

Goal

The goal of this PR is to lay groundwork for the new Provider registration flow by setting up routes for going directly to the become-a-provider route.

Edit Filed issue #194 for looking at a non-hack fix for broken tests.

Related to #184

Discussion

Features:

  • The URL may include the name of the network to connect to. If so, it will try to connect to the given network.
  • The network selection dropdown goes away (on Provider Login and Become a Provider pages) when a network is selected. A 'Change network' link is provided which resets the component.
  • The network part of the URL is optional.

So the following routes all work, and they are case-insensitive.

  1. /become-a-provider: which displays the network dropdown as before
  2. /become-a-provider/mainnet : connects to mainnet immediately from the page
  3. /become-a-provider/testnet: connects to Paseo testnet
  4. /become-a-provider/localhost : tries to connect to localhost:9944
  5. become-a-provider/MaInNet, etc.
  6. /mainnet, /testnet, /localhost will each connect to the main page, select the appropriate network from the dropdown as above, and try to connect to it.
  7. Custom network connections work as before.

Checklist

  • routes for become-a-provider
  • routes for root (provider login)
  • PR Self-Review and Commenting
  • Tests added

How To Test the Changes

Check out the branch and run the playwright tests.

git fetch && git co feat/new-registration-path
npx playwright test

Then you can play around with the new feature via npm run dev.

You should be able to click back and forth between Provider Login and Become a Provider pages, selecting things, then clicking Back/Cancel, and the state of the selections and connections should get reset.

You should be able to go to the URLs listed above and either connect automatically, or select from dropdown and connect, then click 'Change network' and the state should also be reset.

* Allow changing networks
* Allow direct navigation to Become A Provider for a given network, which connects to the network
* playwright tests and other tests for same
endpoint?: string;
genesisHash?: string;
}

export const allNetworks = writable<NetworkInfo[]>([
export const defaultNetworks: NetworkInfo[] = [
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this refactor was just to support converting from the path to the NetworkInfo.

@@ -0,0 +1,7 @@
<script>
Copy link
Collaborator Author

@shannonwells shannonwells Nov 7, 2024

Choose a reason for hiding this comment

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

note the path to this file, it uses the match function in src/params/networks.ts to figure out if the route is valid, and then populates $page.params accordingly with a value for network, or not. If it's something like /become-a-provider/foo it's a 404. The double square braces means it's an optional path in Svelte.

  bookmarked
* make same optional path param from root (Provider Login)
@@ -75,6 +75,6 @@ export default defineConfig({
webServer: {
command: 'npm run build && npm run preview',
port: 4173,
reuseExistingServer: !process.env.CI,
reuseExistingServer: false,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Having this set to true breaks tests.

@@ -16,7 +16,7 @@
<SelectNetworkAndAccount
bind:newUser={$user}
accounts={$nonProviderAccountsStore}
accountSelectorTitle="Account Id"
accountSelectorTitle="Select an Account Id"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

to match the Become a Provider prompt

/>
<div class="flex justify-between align-bottom">
<Button id="connect-button" title="Connect" disabled={!canConnect} action={connect} />
<Button id="connect-button" title="Connect To Account" disabled={!canConnect} action={connect} />
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

"Connect" on the button was confusing due to the other text if you connected to the endpoint. So make the button more clear what you're connecting to.

@@ -0,0 +1,3 @@
export const match = (path: string): boolean => {
return (/mainnet|testnet|localhost/i).test(path);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See defaultAccounts pathname

@shannonwells shannonwells marked this pull request as ready for review November 8, 2024 00:01
Copy link

@JoeCap08055 JoeCap08055 left a comment

Choose a reason for hiding this comment

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

Couple of things:

  1. Playwright tests are passing, but other tests appear to be broken (both when I run locally & in pipeline)
  2. The optional path component for the network is interesting; I'm wondering if you considered a more traditional URL query parameter approach, ie ?node=<mainnet|testnet|URL> (similar to the PolkadotJS UI)? Would seemingly allow for custom networks (or even just specific nodes) right in the link, as opposed to just mainnet/testnet/localhost specific node. Just my $0.02.
    ☝️ non-blocking, but tests should def. be fixed

@shannonwells
Copy link
Collaborator Author

The optional path component for the network is interesting; I'm wondering if you considered

I did, but reading through the docs it didn't seem to be the Svelte Way, and once I already had it implemented, I realized how to do it the other way. ¯_(ツ)_/¯

I could be persuaded to redo it and use query params instead.

Co-authored-by: Joe Caputo <joseph.caputo@projectliberty.io>
@shannonwells shannonwells marked this pull request as draft November 8, 2024 17:29
@JoeCap08055
Copy link

Hmmm, maybe out of scope, but... should the page maybe retain your current network choice/connection when you navigate back to Home? As of now it does not appear to.

Example:

  • Navigate to http://localhost:/localhost (or just select a network from the dropdown from the home page)
  • Click "Become a Provider"
  • Navigate back to "Home" using the link on the page

Expected results: Still connected to "localhost" node
Actual results: Required to select a network connection again

@shannonwells
Copy link
Collaborator Author

shannonwells commented Nov 11, 2024

should the page maybe retain your current network choice/connection when you navigate back to Home? As of now it does not appear to.

That's intentional because, IMO, no it shouldn't. If you navigate back to home then you've changed your mind about what you want to do. We can't predict what the user intent was and it's harmless to make them do just one more click to select from the dropdown and reconnect.

Copy link

@JoeCap08055 JoeCap08055 left a comment

Choose a reason for hiding this comment

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

Pulled & ran tests locally & explored in browser.
Looks good!

@shannonwells shannonwells merged commit 00be832 into main Nov 11, 2024
5 checks passed
@shannonwells shannonwells deleted the feat/new-registration-path branch November 11, 2024 18:40
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.

2 participants