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: APP-243 disable create post button in keplr mobile #2433

Merged
merged 13 commits into from
Sep 10, 2024

Conversation

r41ph
Copy link
Contributor

@r41ph r41ph commented Aug 15, 2024

Description

https://regennetwork.atlassian.net/browse/APP-243


Author Checklist

  • provided a link to the relevant issue or specification
  • provided instructions on how to test
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

How to test

  1. Check on a mobile phone that when logged in with Keplr, the Create Post buttons in the admin's projects page are not visible.

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items
.

I have...

  • confirmed all author checklist items have been addressed
  • reviewed code correctness and readability
  • verified React components follow DRY principles
  • reviewed documentation is accurate
  • reviewed tests
  • manually tested (if applicable)

Copy link

netlify bot commented Aug 15, 2024

Deploy Preview for regen-website ready!

Name Link
🔨 Latest commit a6e4788
🔍 Latest deploy log https://app.netlify.com/sites/regen-website/deploys/66e081b4c0f698000874903e
😎 Deploy Preview https://deploy-preview-2433--regen-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@r41ph r41ph requested review from blushi and flagrede August 19, 2024 15:44
@r41ph
Copy link
Contributor Author

r41ph commented Aug 19, 2024

@erikalogie @clevinson see testing instructions

@erikalogie
Copy link
Collaborator

I can't seem to sign the transaction to actually log in. I tried restarting my phone and same issue.
https://github.com/user-attachments/assets/3706f5cc-5c7d-4b7d-802a-876288c78fe4

@r41ph
Copy link
Contributor Author

r41ph commented Aug 20, 2024

I have the same problem.

@erikalogie
Copy link
Collaborator

I have the same problem.

Do we need to open a separate task to de-bug that in order to test this one? @r41ph

@r41ph
Copy link
Contributor Author

r41ph commented Aug 22, 2024

I'm not sure. On mobile I can login into dev.app.regen.network but not into the deploy previews.. is this known/expected @blushi ?

@blushi
Copy link
Member

blushi commented Aug 22, 2024

I'm not sure. On mobile I can login into dev.app.regen.network but not into the deploy previews.. is this known/expected @blushi ?

Are you trying to log in with email/google or wallet connect?
If using Wallet Connect to connect on mobile (FYI this is not logging in strictly speaking, just connecting to the wallet address), this is expected, we need to add the deploy preview domain on WC cloud: https://cloud.walletconnect.com/app/e828f750-7845-4bae-a9b3-750257ec1b10/project/76798391-210d-47db-a5d1-58c3e83a4a9b/domains, I'll send you an invite

@erikalogie
Copy link
Collaborator

I'm not sure. On mobile I can login into dev.app.regen.network but not into the deploy previews.. is this known/expected @blushi ?

Are you trying to log in with email/google or wallet connect? If using Wallet Connect to connect on mobile (FYI this is not logging in strictly speaking, just connecting to the wallet address), this is expected, we need to add the deploy preview domain on WC cloud: https://cloud.walletconnect.com/app/e828f750-7845-4bae-a9b3-750257ec1b10/project/76798391-210d-47db-a5d1-58c3e83a4a9b/domains, I'll send you an invite

I was logging in with Wallet Connect on my android device

@blushi
Copy link
Member

blushi commented Aug 26, 2024

I'm not sure. On mobile I can login into dev.app.regen.network but not into the deploy previews.. is this known/expected @blushi ?

Are you trying to log in with email/google or wallet connect? If using Wallet Connect to connect on mobile (FYI this is not logging in strictly speaking, just connecting to the wallet address), this is expected, we need to add the deploy preview domain on WC cloud: https://cloud.walletconnect.com/app/e828f750-7845-4bae-a9b3-750257ec1b10/project/76798391-210d-47db-a5d1-58c3e83a4a9b/domains, I'll send you an invite

I was logging in with Wallet Connect on my android device

I've added the deploy preview domain to the allowed ones in Wallet Connect cloud, so you should be able to login now.

Copy link
Member

@blushi blushi left a comment

Choose a reason for hiding this comment

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

we also want to hide the "edit project" button in this case, which doesn't seem to be handled here

@@ -33,7 +33,7 @@ const MyProjects = (): JSX.Element => {
useDashboardContext();
const { track } = useTracker();
const [projectsCurrentStep] = useAtom(projectsCurrentStepAtom);
const { wallet, loginDisabled } = useWallet();
const { wallet, loginDisabled, isKeplrMobileWeb } = useWallet();
Copy link
Member

Choose a reason for hiding this comment

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

isKeplrMobileWeb is not the right variable to use here.
It indicates whether you're connected from the Keplr mobile app itself (from their in-app browser) or not.
Instead, we want to use loginDisabledwhich will be true in those cases:

  • connected through wallet connect from mobile
  • connected through wallet connect from desktop
  • connected from the keplr mobile in-app browser (but we can't really test this with deploy previews since it points to the production app)

@blushi blushi changed the title feat: hide create post button in keplr mobile feat: APP-243 hide create post button in keplr mobile Aug 26, 2024
@r41ph r41ph force-pushed the feat-APP-243-hide-create-post-buttons-in-keplr-mobile branch from 867e809 to 5b24b57 Compare August 27, 2024 12:22
@r41ph r41ph requested a review from blushi August 27, 2024 12:22
@blushi blushi removed the request for review from flagrede August 27, 2024 14:52
Copy link
Member

@blushi blushi left a comment

Choose a reason for hiding this comment

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

so the "edit project" button is greyed out while the "create post" button is completely hidden
should we just hide all buttons? cc/ @erikalogie

IMG_2D79C5DBEF2C-1

@erikalogie
Copy link
Collaborator

so the "edit project" button is greyed out while the "create post" button is completely hidden should we just hide all buttons? cc/ @erikalogie

IMG_2D79C5DBEF2C-1

Maybe instead it would be better to add a tooltip on rollover of the button that explains why editing isn't possible? Even I forgot and got confused while testing. Something like "Editing this project requires signing a transaction not supported by Keplr mobile. Please log in on desktop to edit this project." Does that tooltip text make sense to you @blushi?

@blushi
Copy link
Member

blushi commented Aug 27, 2024

so the "edit project" button is greyed out while the "create post" button is completely hidden should we just hide all buttons? cc/ @erikalogie
IMG_2D79C5DBEF2C-1

Maybe instead it would be better to add a tooltip on rollover of the button that explains why editing isn't possible? Even I forgot and got confused while testing. Something like "Editing this project requires signing a transaction not supported by Keplr mobile. Please log in on desktop to edit this project." Does that tooltip text make sense to you @blushi?

That could be helpful indeed, I'll think a bit more about the wording.
Should we do the same for the "create post" button then instead of completely hiding it?

@blushi
Copy link
Member

blushi commented Aug 28, 2024

so the "edit project" button is greyed out while the "create post" button is completely hidden should we just hide all buttons? cc/ @erikalogie
IMG_2D79C5DBEF2C-1

Maybe instead it would be better to add a tooltip on rollover of the button that explains why editing isn't possible? Even I forgot and got confused while testing. Something like "Editing this project requires signing a transaction not supported by Keplr mobile. Please log in on desktop to edit this project." Does that tooltip text make sense to you @blushi?

That could be helpful indeed, I'll think a bit more about the wording. Should we do the same for the "create post" button then instead of completely hiding it?

Thinking about having consistent behavior throughout the app regarding that feature limitation. Right now, we also just hide the "edit profile" nav item. If we want to be consistent, we would also have this nav item disabled instead and show a tooltip on rollover.
Regarding the tooltip text, I believe we might not need to go into the details of why this is not supported yet. Also you could connect on desktop with Wallet Connect so we should be a bit more precise. So I think we could say instead "This is not yet supported by Wallet Connect and Keplr mobile. Please log in using Keplr browser extension on desktop".

@blushi
Copy link
Member

blushi commented Aug 29, 2024

so the "edit project" button is greyed out while the "create post" button is completely hidden should we just hide all buttons? cc/ @erikalogie
IMG_2D79C5DBEF2C-1

Maybe instead it would be better to add a tooltip on rollover of the button that explains why editing isn't possible? Even I forgot and got confused while testing. Something like "Editing this project requires signing a transaction not supported by Keplr mobile. Please log in on desktop to edit this project." Does that tooltip text make sense to you @blushi?

That could be helpful indeed, I'll think a bit more about the wording. Should we do the same for the "create post" button then instead of completely hiding it?

Thinking about having consistent behavior throughout the app regarding that feature limitation. Right now, we also just hide the "edit profile" nav item. If we want to be consistent, we would also have this nav item disabled instead and show a tooltip on rollover. Regarding the tooltip text, I believe we might not need to go into the details of why this is not supported yet. Also you could connect on desktop with Wallet Connect so we should be a bit more precise. So I think we could say instead "This is not yet supported by Wallet Connect and Keplr mobile. Please log in using Keplr browser extension on desktop".

@erikalogie any thoughts on this?

@erikalogie
Copy link
Collaborator

so the "edit project" button is greyed out while the "create post" button is completely hidden should we just hide all buttons? cc/ @erikalogie
IMG_2D79C5DBEF2C-1

Maybe instead it would be better to add a tooltip on rollover of the button that explains why editing isn't possible? Even I forgot and got confused while testing. Something like "Editing this project requires signing a transaction not supported by Keplr mobile. Please log in on desktop to edit this project." Does that tooltip text make sense to you @blushi?

That could be helpful indeed, I'll think a bit more about the wording. Should we do the same for the "create post" button then instead of completely hiding it?

Thinking about having consistent behavior throughout the app regarding that feature limitation. Right now, we also just hide the "edit profile" nav item. If we want to be consistent, we would also have this nav item disabled instead and show a tooltip on rollover. Regarding the tooltip text, I believe we might not need to go into the details of why this is not supported yet. Also you could connect on desktop with Wallet Connect so we should be a bit more precise. So I think we could say instead "This is not yet supported by Wallet Connect and Keplr mobile. Please log in using Keplr browser extension on desktop".

I think this is a good idea just to avoid confusion. I like your suggestion for the tooltip too.

@blushi
Copy link
Member

blushi commented Aug 29, 2024

so the "edit project" button is greyed out while the "create post" button is completely hidden should we just hide all buttons? cc/ @erikalogie
IMG_2D79C5DBEF2C-1

Maybe instead it would be better to add a tooltip on rollover of the button that explains why editing isn't possible? Even I forgot and got confused while testing. Something like "Editing this project requires signing a transaction not supported by Keplr mobile. Please log in on desktop to edit this project." Does that tooltip text make sense to you @blushi?

That could be helpful indeed, I'll think a bit more about the wording. Should we do the same for the "create post" button then instead of completely hiding it?

Thinking about having consistent behavior throughout the app regarding that feature limitation. Right now, we also just hide the "edit profile" nav item. If we want to be consistent, we would also have this nav item disabled instead and show a tooltip on rollover. Regarding the tooltip text, I believe we might not need to go into the details of why this is not supported yet. Also you could connect on desktop with Wallet Connect so we should be a bit more precise. So I think we could say instead "This is not yet supported by Wallet Connect and Keplr mobile. Please log in using Keplr browser extension on desktop".

I think this is a good idea just to avoid confusion. I like your suggestion for the tooltip too.

@r41ph it's becoming a bit out of scope of the initial issue, would you rather address adding the tooltip and dealing with "edit profile" in another task? or should we do it all as part of this one?

In any case, here we would want to disable "create post" instead of completely hiding it.

@blushi
Copy link
Member

blushi commented Sep 2, 2024

We've discussed this with @r41ph and agreed to handle the user menu items (edit profile and settings) in a separate task.

@erikalogie
Copy link
Collaborator

Sounds good! Can someone put that task into the upcoming sprint?

@blushi
Copy link
Member

blushi commented Sep 3, 2024

Sounds good! Can someone put that task into the upcoming sprint?

done: https://regennetwork.atlassian.net/jira/software/c/projects/ENG/boards/51?selectedIssue=APP-330

@r41ph
Copy link
Contributor Author

r41ph commented Sep 3, 2024

Ok cool. Summarising: Both Create Post and Edit Project buttons will be greyed out/disabled and both will have a tooltip when users log in using Wallet Connect on mobile or Keplr mobile, right? Also, point out that on mobile the tooltips will open only when the user clicks the buttons. @erikalogie @blushi

@blushi
Copy link
Member

blushi commented Sep 3, 2024

Ok cool. Summarising: Both Create Post and Edit Project buttons will be greyed out/disabled and both will have a tooltip when users log in using Wallet Connect on mobile or Keplr mobile, right? Also, point out that on mobile the tooltips will open only when the user clicks the buttons. @erikalogie @blushi

SGTM

@erikalogie
Copy link
Collaborator

LGTM, only thing (which can be separate issue) is that the "edit" icon seems to disappear here when "edit project" is disabled. Should be white icon on the grey so it is still viewable.

@r41ph r41ph force-pushed the feat-APP-243-hide-create-post-buttons-in-keplr-mobile branch from c670e7f to c20a0cb Compare September 4, 2024 15:29
@r41ph r41ph requested a review from blushi September 4, 2024 15:29
@r41ph r41ph force-pushed the feat-APP-243-hide-create-post-buttons-in-keplr-mobile branch from c20a0cb to d254475 Compare September 4, 2024 16:01
@r41ph
Copy link
Contributor Author

r41ph commented Sep 4, 2024

@erikalogie the icon should be visible now

@erikalogie
Copy link
Collaborator

Great, LGTM!

@r41ph r41ph requested a review from blushi September 6, 2024 11:32
@r41ph r41ph force-pushed the feat-APP-243-hide-create-post-buttons-in-keplr-mobile branch from 77aab08 to b3613d8 Compare September 6, 2024 11:32
Copy link
Member

@blushi blushi left a comment

Choose a reason for hiding this comment

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

While connected with Wallet Connect, for any project card, the "create post" tooltip shows the one telling it's a draft or has no location even though it's not always the case. We should rather show the tooltip related to WC/Keplr mobile in this case.

IMG_4406

@r41ph r41ph force-pushed the feat-APP-243-hide-create-post-buttons-in-keplr-mobile branch from b3613d8 to 796e196 Compare September 10, 2024 09:37
@r41ph r41ph requested a review from blushi September 10, 2024 09:41
Copy link
Member

@blushi blushi left a comment

Choose a reason for hiding this comment

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

tACK, pre-approving, pending small nit

@@ -2,6 +2,7 @@ import { msg } from '@lingui/macro';

export const DRAFT_ID = 'draft';
export const CREATE_POST = msg`+ create post`;
export const CREATE_POST_DISABLED_TOOLTIP = msg`You cannot make posts because either the project is still a draft or it doesn't have assigned a location.`;
export const CREATE_POST_TOOLTIP_TEXT = msg`You cannot make posts because either the project is still a draft or it doesn't have assigned a location.`;
export const EDIT_PROJECT_TOOLTIP_TEXT = msg`This is not yet supported by Wallet Connect and Keplr mobile. Please log in using Keplr browser extension on desktop.`;
Copy link
Member

Choose a reason for hiding this comment

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

maybe we could rename this to NOT_SUPPORTED_TOOLTIP_TEXT or something similar since this is not just about editing a project

@r41ph r41ph force-pushed the feat-APP-243-hide-create-post-buttons-in-keplr-mobile branch from 77499a3 to a6e4788 Compare September 10, 2024 17:28
@r41ph r41ph merged commit df0908f into dev Sep 10, 2024
14 checks passed
@r41ph r41ph deleted the feat-APP-243-hide-create-post-buttons-in-keplr-mobile branch September 10, 2024 17:32
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.

3 participants