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

No more default protocol-redirect #107

Merged
merged 11 commits into from
Nov 1, 2023
Merged

No more default protocol-redirect #107

merged 11 commits into from
Nov 1, 2023

Conversation

bumi
Copy link
Contributor

@bumi bumi commented Jul 10, 2023

as more apps are implementing NWC now it does not properly work anymore to open the correct app there. the default app is no longer the requested app.

We add the option for a button to open the nostr+walletconnect link, allow users to copy the NWC string AND also fire events on window and the window.opener to allow apps to listen to those events (especially in when apps open the NWC confirmation URL or use an embedded webview.

e.g. zapplepay

Missing:

  • better UI to copy the connection string
  • better UI with link to open the connection string

as more apps are implementing NWC now it does not properly work anymore to open the correct app there.
the default app is no longer the used app
@bumi bumi requested review from kiwiidb, rolznz and reneaaron July 10, 2023 16:40
@stackingsaunter
Copy link
Contributor

This allows apps that embed a webview for example to enable the connection
to listen to this event an know when the connection was successfuly enabled.
@bumi
Copy link
Contributor Author

bumi commented Oct 5, 2023

actually I don't think it feels good.
all other pages are more or less full width and here we have this small card now.

CleanShot 2023-10-05 at 15 44 01@2x

maybe just like this:
CleanShot 2023-10-05 at 15 46 19@2x

do we need a back button?
I also think the open in app should not be a main call to action button. In many cases this will not work imo.

@rolznz
Copy link
Contributor

rolznz commented Oct 6, 2023

do we need a back button?

What does the back button do? the connection is already created. The normal browser button takes you back to the create connection screen but some of the form fields are not preserved (like the connection name).

Also, the connection is already created at that point, so going back is going to leave an unused connection for the user. I don't think that's very good.

@rolznz
Copy link
Contributor

rolznz commented Oct 6, 2023

image
I think the buttons don't look so good stretched like that. Maybe add a div to wrap the buttons with a sensible max-width?

{{if not (eq .PairingSecret "")}}
<p class="mb-4 dark:text-white">Use the pairing secret to connect to your app.</p>
<a href="{{.PairingUri}}" class="w-full inline-flex bg-purple-700 cursor-pointer duration-150 focus:outline-none font-medium hover:bg-purple-900 items-center justify-center px-3 py-2 rounded-md shadow text-white transition mb-4">
<svg class="mr-2" width="24" height="24" viewBox="0 0 24 24" fill="none" xmlns="http://www.w3.org/2000/svg">
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should directly embed svgs like this, what was the reason for the change?

Copy link
Member

Choose a reason for hiding this comment

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

We can't add a fill to svgs if we use them from the img tag

Copy link
Contributor

Choose a reason for hiding this comment

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

@im-adithya what about a template, like {{template "copyIcon" .}} and then move the copy icon to a separate html file?

Copy link
Member

Choose a reason for hiding this comment

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

Not very sure about that, it works, but then again we have to mention each and every icon used in that file in echo_handlers.go to parse it

@im-adithya
Copy link
Member

do we need a back button?

I also felt at first but what Roland says makes sense, but it still expects us to click "Connections" from the navbar to go back which somehow doesn't feel right.

And also if we place a back button we might also want to change the wording "Pair with app to finalize" because it doesn't look like the creation is done yet and might confuse into thinking they're aborting it (which is definitely not the case)

(Another way to do is to delete the connection when back is pressed which is unnecessarily complicated)

@im-adithya im-adithya mentioned this pull request Oct 12, 2023
1 task
@reneaaron reneaaron merged commit 570fbaa into main Nov 1, 2023
2 checks passed
@reneaaron reneaaron deleted the no-protocol-redirect branch November 2, 2023 13:23
bumi added a commit that referenced this pull request Nov 25, 2023
# By Roland Bewick (20) and others
# Via GitHub (13) and others
* main: (38 commits)
  chore: add dependabot
  feat: add support both internal and public relay URL
  fix: relay public url
  fix: expiresAt and maxAmount handling
  fix: app expiresAt handling
  fix: style for darkmode
  chore: correctly handle query parameters in new UI (WIP)
  chore: rename app name parameter (#154)
  fix: navbar padding on lg
  fix: ui cleanup
  no more default protocol-redirect (#107)
  ui improvements & layout simplification (#153)
  feat: nwc connection page ui (#151)
  chore: address migration feedback
  chore: format with prettier
  feat: run css scripts via npm
  chore: add human-readable name to migration ids
  feat: add manual migrations using gormigration
  fix: don't log resp id
  fix: convert expiresAt to int
  ...

# Conflicts:
#	main.go
bumi added a commit that referenced this pull request Nov 25, 2023
* feature/breez-backend: (38 commits)
  chore: add dependabot
  feat: add support both internal and public relay URL
  fix: relay public url
  fix: expiresAt and maxAmount handling
  fix: app expiresAt handling
  fix: style for darkmode
  chore: correctly handle query parameters in new UI (WIP)
  chore: rename app name parameter (#154)
  fix: navbar padding on lg
  fix: ui cleanup
  no more default protocol-redirect (#107)
  ui improvements & layout simplification (#153)
  feat: nwc connection page ui (#151)
  chore: address migration feedback
  chore: format with prettier
  feat: run css scripts via npm
  chore: add human-readable name to migration ids
  feat: add manual migrations using gormigration
  fix: don't log resp id
  fix: convert expiresAt to int
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants