search

sporeprotocol / spore-demo

A Spore Protocol Demo based on Next.js + React + Spore SDK
https://a-simple-demo.spore.pro
MIT License
3 stars 8 forks source link

fix: disabled JoyID connector #68

Closed ahonn closed 7 months ago

ahonn commented 7 months ago

See https://github.com/sporeprotocol/spore-demo/issues/67 and https://github.com/sporeprotocol/spore-demo/issues/69

vercel[bot] commented 7 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
spore-demo ✅ Ready (Inspect) Visit Preview Feb 8, 2024 4:16am
ShookLyngs commented 7 months ago

Hi, I have a question: why not remove or comment out the new JoyIdConnector() from the connectors? https://github.com/sporeprotocol/spore-demo/blob/0d95ee86662be316fa80970360d01a3f59106823/src/pages/_app.tsx#L95-L98

ahonn commented 7 months ago

Hi, I have a question: why not remove it from the JoyIdConnector from the connectors?

https://github.com/sporeprotocol/spore-demo/blob/0d95ee86662be316fa80970360d01a3f59106823/src/pages/_app.tsx#L95-L98

For users who have previously used JoyID connect, removing the JoyId connector directly may result in errors and unexpected inability to control Spore that was previously Minted. Therefore, I am more inclined to only hide it for new users without affecting existing users who use JoyID for connect.

ShookLyngs commented 7 months ago

For users who have previously used JoyID connect, removing the JoyId connector directly may result in errors and unexpected inability to control Spore that was previously Minted. Therefore, I am more inclined to only hide it for new users without affecting existing users who use JoyID for connect.

Okay, now I understand. This is a simple change to remove the JoyID option from the wallet connect modal without affecting other users who have already connected via JoyID. However, if the connected user accidentally clicks the "disconnect" button, does it mean that the user will lose the ability to reconnect via JoyID thereafter?

For that, I recommend marking the connector as somewhat "deprecated" and still showing it in the wallet connect modal, but lowering its ordering. Additionally, add some text to explain why this option is currently not recommended. Do you think it would be a better approch for everyone?

ahonn commented 7 months ago

For users who have previously used JoyID connect, removing the JoyId connector directly may result in errors and unexpected inability to control Spore that was previously Minted. Therefore, I am more inclined to only hide it for new users without affecting existing users who use JoyID for connect.

Okay, now I understand. This is a simple change to remove the JoyID option from the wallet connect modal without affecting other users who have already connected via JoyID. However, if the connected user accidentally clicks the "disconnect" button, does it mean that the user will lose the ability to reconnect via JoyID thereafter?

For that, I recommend marking the connector as somewhat "deprecated" and still showing it in the wallet connect modal, but lowering its ordering. Additionally, add some text to explain why this option is currently not recommended. Do you think it would be a better approch for everyone?

There are currently only two connector available, so simply marking it as deprecated may not be sufficient. My idea is to discourage future users from using the JoyID connector. The current changes has minimal impact.

I believe it is acceptable that existing users who disconnect from JoyID connect will no longer be able to use it, as the purpose of this change is to prevent any confusion for them. In the README, I have also clearly stated that we have temporarily hidden the integration of JoyID.

Perhaps we can consider show tips when existing users disconnect from JoyID connect, but I believe the necessity of doing so is relatively low. What do you think?

ShookLyngs commented 7 months ago

Perhaps we can consider show tips when existing users disconnect from JoyID connect, but I believe the necessity of doing so is relatively low. What do you think?

From my perspective, I believe it would be more reasonable to add a card/box in the ConnectModal to explain why the JoyID option is no longer visible (like you did in the README). This approach ensures that users understand the change and discourages them from attempting to connect using JoyID again, not when they're trying to disconnect.

ahonn commented 7 months ago

Perhaps we can consider show tips when existing users disconnect from JoyID connect, but I believe the necessity of doing so is relatively low. What do you think?

From my perspective, I believe it would be more reasonable to add a card/box in the ConnectModal to explain why the JoyID option is no longer visible. This approach ensures that users understand the change and discourages them from attempting to connect using JoyID again, not when they're trying to disconnect.

Cool, I think adding a message in ConnectModal to provide an explanation is better.

ahonn commented 7 months ago

@ShookLyngs Now we can still see JoyID in ConnectModal, but it is marked as disabled. We will display a message about JoyID being deprecated and link to https://github.com/sporeprotocol/spore-demo/issues/69

image

ShookLyngs commented 7 months ago

@ShookLyngs Now we can still see JoyID in ConnectModal, but it is marked as disabled. We will display a message about JoyID being deprecated and link to #69

Cool, I have a question: do you think it's a good idea to change the ordering of the elements in the modal? I feel like the current ordering (Message -> MetaMask -> JoyID) might hinder MetaMask-only users as they only want to connect via MetaMask.

Would it be better if we change the order to the following:

1. MetaMask
2. JoyID
3. Message
ahonn commented 7 months ago

@ShookLyngs Now we can still see JoyID in ConnectModal, but it is marked as disabled. We will display a message about JoyID being deprecated and link to #69

Cool, I have a question: do you think it's a good idea to change the ordering of the elements in the modal? I feel like the current ordering (Message -> MetaMask -> JoyID) might hinder MetaMask-only users as they only want to connect via MetaMask.

Would it be better if we change the order to the following:

1. MetaMask
2. JoyID
3. Message

Usually, announcements or notifications should be placed at the top, so I have followed this convention. I don’t have a particular preference for where to place it, I think both options are fine.

ShookLyngs commented 7 months ago

Usually, announcements or notifications should be placed at the top, so I have followed this convention. I don’t have a particular preference for where to place it, I think both options are fine.

Yeah, I think if you wish the message to be eye-catching, the current ordering is just corret. I have no other questions for it, thanks.