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-201 buy credits flow step 1 #2408

Open
wants to merge 145 commits into
base: dev
Choose a base branch
from

Conversation

r41ph
Copy link
Contributor

@r41ph r41ph commented Jul 22, 2024

Description

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

  • Implementation in Storybook of the UI for the "buy credits flow step 1"
  • Migration to Vitest in web-marketplace and web-components
  • Some components have been refactored to reuse them for this issue.
  • Remove /web-auth workspace
  • Fix Buffer is not defined error in Storybook, closes: Fix Buffer issue on some stories rnd-dev-team#1799

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • 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

No wrapper component for the whole step 1 has been implemented for now (will be in a follow-up integration task)

Individual components to review:
https://deploy-preview-2408--regen-storybook.netlify.app/?path=/story/denomiconwithcurrency--icon-and-currency
https://deploy-preview-2408--regen-storybook.netlify.app/?path=/story/cards-ordersummarycard--default
https://deploy-preview-2408--regen-storybook.netlify.app/?path=/story/denomiconwithcurrency--with-tooltip
https://deploy-preview-2408--regen-storybook.netlify.app/?path=/story/supcurrencyandamount--default

Fixed stories
https://deploy-preview-2408--regen-storybook.netlify.app/?path=/story/marketplace-organisms-choosecreditsform--choose-credits
https://deploy-preview-2408--regen-storybook.netlify.app/?path=/story/marketplace-molecules-creditsamount--credits-amount-card
https://deploy-preview-2408--regen-storybook.netlify.app/?path=/story/marketplace-molecules-creditsamount--credits-amount-crypto

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 Jul 22, 2024

Deploy Preview for regen-website ready!

Name Link
🔨 Latest commit 88997ab
🔍 Latest deploy log https://app.netlify.com/sites/regen-website/deploys/66df21aeeee7320008e7e777
😎 Deploy Preview https://deploy-preview-2408--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 a review from blushi July 22, 2024 19:31
@blushi blushi changed the title Feat app 201 buy credits flow step 1 feat: APP-201 buy credits flow step 1 Jul 25, 2024
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.

I've went through half of the PR, will continue next week.
Could you add some documentation about the tests setup you added? In the root README

web-marketplace/src/components/molecules/Form/Form.tsx Outdated Show resolved Hide resolved
tailwind.common.js Outdated Show resolved Hide resolved
tailwind.css Outdated Show resolved Hide resolved
web-components/src/components/buttons/EditButtonIcon.tsx Outdated Show resolved Hide resolved
web-components/src/components/buttons/TextButton.tsx Outdated Show resolved Hide resolved
@blushi
Copy link
Member

blushi commented Jul 25, 2024

@erikalogie see testing instructions
a fix will be needed before you can test some of the stories as mentioned in the PR description

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.

For crypto purchase, when selecting a currency, it should say "1000 credits available in ${selectedCurrency}" because the number of credits available won't be the same based on the selected currency since for each sell order, the seller specifies one "ask denom": https://www.figma.com/design/BTuUv6QXY4GbliZcXCe8RJ/Fiat-payments?node-id=67-55906&t=HKrNrjQMGmY10ZLM-0

image

@erikalogie
Copy link
Collaborator

Cards : OrderSummaryCard - With Payment Details ⋅ Storybook 2024-07-25 16-02-24

A few small pieces of feedback, overall looks great

@r41ph r41ph force-pushed the feat-APP-201-buy-credits-flow-step-1 branch from 504d17c to b184ab7 Compare August 1, 2024 16:31
@r41ph
Copy link
Contributor Author

r41ph commented Aug 1, 2024

@blushi @erikalogie All requested updates have been implemented, but we are still getting the error in some components in Storybook, so still WIP. For details on this see #2408 (comment)

@erikalogie
Copy link
Collaborator

Ok great. This looks good. If it is possible to fix the baselines of the text so they all align, that would be awesome.

Cards : OrderSummaryCard - Default ⋅ Storybook 2024-08-05 08-26-32

@r41ph
Copy link
Contributor Author

r41ph commented Aug 5, 2024

@erikalogie the issue with Storybook has been fixed. See links to fixed stories in the description please.

@r41ph r41ph requested a review from blushi August 5, 2024 14:25
@erikalogie
Copy link
Collaborator

erikalogie commented Aug 6, 2024

Looks good!

Some small feedback, this is my fault for not providing a mobile view of the crypto view:

  1. Fix this issue with the amount:
    organisms : ChooseCreditsForm - Choose Credits Step 1 ⋅ Storybook 2024-08-06 09-35-22.
  2. Make some small tweaks to the sizing of the fonts in the crypto purchase and advanced settings sections on mobile, including the space between the sections I think would look better if it were less.

This is the rollover state for the unselected tab: https://www.figma.com/design/BTuUv6QXY4GbliZcXCe8RJ/Fiat-payments?node-id=2874-106112&t=bidt1T6CnWSUAMI3-1

This should get fixed:
organisms : ChooseCreditsForm - Choose Credits Step 1 ⋅ Storybook 2024-08-06 09-46-32

@erikalogie
Copy link
Collaborator

organisms : ChooseCreditsForm - Choose Credits Step 1 ⋅ Storybook 2024-08-07 13-34-32 One last thing, if you could remove the "in USD" from the "buy with credit card" version

@r41ph r41ph force-pushed the feat-APP-201-buy-credits-flow-step-1 branch from 43d39ce to 7a61b7c Compare August 8, 2024 08:47
@r41ph r41ph requested a review from blushi August 8, 2024 09:01
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.

Nice work! Still a few nits.

Thanks for adding all the tests. it'd be nice to give a small presentation of that sometimes during the architecture call so we start using those more widely and decide what's the strategy for adding tests to existing components.

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.

LGTM, pre-approving, pending fixing the TS error in web-marketplace that cause CI to fail and @erikalogie's QA approval

@r41ph r41ph force-pushed the feat-APP-201-buy-credits-flow-step-1 branch from 69d5a28 to 5596739 Compare August 8, 2024 12:57
@blushi
Copy link
Member

blushi commented Aug 26, 2024

@erikalogie could you have a final look at this?

Ralph added 29 commits September 9, 2024 17:25
@r41ph r41ph force-pushed the feat-APP-201-buy-credits-flow-step-1 branch from 7e3d392 to 88997ab Compare September 9, 2024 16:26
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.

Fix Buffer issue on some stories
3 participants