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

Crypto Payment #1844

Merged
merged 31 commits into from
Jan 18, 2022
Merged

Crypto Payment #1844

merged 31 commits into from
Jan 18, 2022

Conversation

FSM1
Copy link
Contributor

@FSM1 FSM1 commented Jan 7, 2022

closes #1782


Submission checklist:

Layout

  • Change looks good in the desktop web ui
  • Change looks good in the mobile web ui

Theme

  • Components / elements inspected in light mode
  • Components / elements inspected in dark mode

@render
Copy link

render bot commented Jan 7, 2022

@render
Copy link

render bot commented Jan 7, 2022

@FSM1 FSM1 requested a review from Tbaut January 7, 2022 15:59
@render
Copy link

render bot commented Jan 7, 2022

@FSM1 FSM1 requested review from tanmoyAtb and removed request for Tbaut January 7, 2022 15:59
@github-actions github-actions bot added the Type: Feature Added to PRs to identify that the change is a new feature. label Jan 7, 2022
@FSM1 FSM1 requested a review from Tbaut January 7, 2022 15:59
@FSM1 FSM1 self-assigned this Jan 7, 2022
@FSM1 FSM1 added the Status: Review Needed 👀 Added to PRs when they need more review label Jan 7, 2022
Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

Went through the code, and played a bit, looks great already! Left a couple comments.

Copy link
Contributor

@tanmoyAtb tanmoyAtb left a comment

Choose a reason for hiding this comment

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

Overall implementation looks great and well connected.
Left some comments. It would be great to have the loaders in place.

Some of the UI could use some spacing in between,
between QR code and text,
Screenshot 2022-01-11 at 10 23 34 PM

below the select cryptocurrency label,
Screenshot 2022-01-11 at 10 29 58 PM

The dark mode and mobile views need some tweaks.

One thing that happens,
Once I initialize a crypto payment, if I go through the select plan again, I get the Failed to create a charge error. We should probably disable plan change while an invoice is pending. Ok to have this in scope of the previous invoices issue #1829

FSM1 and others added 4 commits January 11, 2022 22:40
Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
Co-authored-by: Tanmoy Basak Anjan <tanmoy3399@gmail.com>
@Tbaut Tbaut requested review from Tbaut and removed request for tanmoyAtb January 13, 2022 15:55
Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

I was on the Plan 2 out of 3 (monthy payment), and to test the crypto payment I click on the plan 3.
Now I never have the option to pay annually. Isn't that an issue?

Billing-weird-2.mp4

@Tbaut
Copy link
Collaborator

Tbaut commented Jan 13, 2022

Yeah there's something very weird, the plan 3 doesn't allow the annual/monthly payment 🤷 The plan 2 does.

annual-missing.mp4

@Tbaut
Copy link
Collaborator

Tbaut commented Jan 13, 2022

Once I initialize a crypto payment, if I go through the select plan again, I get the Failed to create a charge error. We should probably disable plan change while an invoice is pending. Ok to have this in scope of the previous invoices issue #1829

I ran into this too when I clicked on "go back" as I was on the crypto payment slide

@Tbaut
Copy link
Collaborator

Tbaut commented Jan 13, 2022

The dark view looks good, but the mobile view modal is to wide and on iphone 11 pro, or Galaxy S20 (pretty recent phone I believe) it's not scaling well:
image
image

Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

A couple nits that the linter doesn't figure out 🙄

@FSM1
Copy link
Contributor Author

FSM1 commented Jan 14, 2022

The dark view looks good, but the mobile view modal is to wide and on iphone 11 pro, or Galaxy S20 (pretty recent phone I believe) it's not scaling well

I suspect this only happens when the viewport size changes during rendering. At least it seems like it is fine when the page is rendered with mobile emulation on from the start:
image

@FSM1
Copy link
Contributor Author

FSM1 commented Jan 14, 2022

I was on the Plan 2 out of 3 (monthy payment), and to test the crypto payment I click on the plan 3. Now I never have the option to pay annually. Isn't that an issue?

This is merely a side-effect of the daily/annual selector, since the Premium plan does not have a daily price, which is what is being looked for by the code... Once we revert back to the annual period this should return to normal.

@Tbaut
Copy link
Collaborator

Tbaut commented Jan 14, 2022

suspect this only happens when the viewport size changes during rendering. At least it seems like it is fine when the page is rendered with mobile emulation on from the start:

I just tried, and it's the same, try with the Galaxy S20 non ultra, you'll see

@FSM1
Copy link
Contributor Author

FSM1 commented Jan 14, 2022

suspect this only happens when the viewport size changes during rendering. At least it seems like it is fine when the page is rendered with mobile emulation on from the start:

I just tried, and it's the same, try with the Galaxy S20 non ultra, you'll see

Ahhh great, brave removed a bunch of devices from their emulation. Either case, I seem to have figured out what was causing these glitches and resolved...

@FSM1 FSM1 requested a review from Tbaut January 14, 2022 16:30
@Tbaut
Copy link
Collaborator

Tbaut commented Jan 14, 2022

Oops the cross isn't happy with that last change, and also the clock for the remaining time I think is messed up.
image

@FSM1
Copy link
Contributor Author

FSM1 commented Jan 17, 2022

Oops the cross isn't happy with that last change, and also the clock for the remaining time I think is messed up. ![image]

I think I have finally nailed down the styling weirdness... @Tbaut would appreciate your keen eye on this one 🙏🏼

Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

Absolutely, you nailed it and I couldn't find any other issue!

@FSM1 FSM1 merged commit c79b2f5 into dev Jan 18, 2022
@FSM1 FSM1 deleted the feat/crypto-payment-1782 branch January 18, 2022 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Review Needed 👀 Added to PRs when they need more review Type: Feature Added to PRs to identify that the change is a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade Plan Flow - Crypto
4 participants