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.

Sign up for GitHub

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

Jump to bottom

feat(ramp): add sell fields to order details view #7742

Merged
merged 58 commits into from
Nov 16, 2023
Merged

feat(ramp): add sell fields to order details view #7742

merged 58 commits into from
Nov 16, 2023

Conversation

georgeweiler
Copy link
Contributor

Description

Adds support on the order details page for sell orders.

Related issues

N/A This is a new feature

Manual testing steps

Add a sell order to your app state and check the order details page

Screenshots/Recordings

Before

After

Screenshot 2023-11-09 at 10 49 01 AM

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've clearly explained what problem this PR is solving and how it is solved.
  • I've linked related issues
  • I've included manual testing steps
  • I've included screenshots/recordings if applicable
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.
  • I’ve properly set the pull request status:
    • In case it's not yet "ready for review", I've set it to "draft".
    • In case it's "ready for review", I've changed it from "draft" to "non-draft".

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

wachunei and others added 30 commits October 17, 2023 15:24
@wachunei
feat(ramp): add confirm button and initial send transaction view
a80577c
@wachunei
feat(ramp): make confirm release animation faster
925a1aa
@wachunei
refactor(ramp): refactor screen layout props
362edfc
@wachunei
feat(ramp): add amount to provider and change send button label
700debf
@wachunei
feat(ramp): add initial transaction screen
3f5c920
@wachunei
refactor(ramp): removed unused button component
4af6d4a
@wachunei
test(ramp): add initial test for transaction view
85c61bd
@wachunei
test(ramp): change rendered screen name
f1b6b14
@wachunei
test(ramp): add send button tests
57f9b69
@wachunei
Merge branch 'feat/off-ramp' into feat/ramp-transaction-view
0e7869f
@wachunei
feat(ramp): add destination and change button label
00f1289
@wachunei
Merge branch 'feat/off-ramp' into feat/ramp-transaction-view
0a0214a
@wachunei
test(ramp): add case for custom action payment method
4cd4383
@wachunei
feat(ramp): add sell tx hash type and redux actions
47d9241
@wachunei
test(ramp): add -1 index or undefined properties cases
5dbca15
@wachunei
Merge branch 'feat/off-ramp' into feat/ramp-transaction-view
d8338ad
@wachunei
Merge branch 'feat/ramp-transaction-view' into feat/ramp-sell-transac…
1b68d57 
…tion-hash
@wachunei
refactor(ramp): improve sell destination styling on tx screen
bb5bdee
@wachunei
Merge branch 'feat/off-ramp' into feat/ramp-transaction-view
809a3c4
@wachunei
Merge branch 'feat/ramp-transaction-view' into feat/ramp-sell-transac…
a0f1119 
…tion-hash
@wachunei
refactor(ramp): remove unneecessary row
8eff8ed
@wachunei
feat(ramp): add sell tx hash when sending sell tx
d697efa
@georgeweiler
refactor(ramp): move OrderDetails to its own directory
65a3ac7
@georgeweiler
feat(ramp): update order details CTA
ac77f2d
@georgeweiler
feat(ramp): updates order details fields for sell orders
bf6fea6
@georgeweiler
feat(ramp): adds error and loading views for created sell orders
351055b
@georgeweiler
fix(ramp): only run the created order useEffect on page mount
67b8bb0
@wachunei
feat(ramp): use localized send string
a78628a
@wachunei
Merge branch 'feat/ramp-transaction-view' into feat/ramp-sell-transac…
648809b 
…tion-hash
@wachunei
Merge branch 'feat/ramp-sell-transaction-hash' into feat/ramp-add-sel…
9775c68 
…l-transaction-hash
wachunei
wachunei approved these changes Nov 14, 2023
View reviewed changes
Copy link
Member

@wachunei wachunei left a comment

Choose a reason for hiding this comment

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

LGTM

@wachunei wachunei removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Nov 14, 2023
@georgeweiler georgeweiler marked this pull request as ready for review November 14, 2023 16:07
@github-actions GitHub Actions
Copy link
Contributor

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/2a856aeb-942a-4879-b059-2b160cb1ac6a
You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

Cal-L
Cal-L reviewed Nov 14, 2023
View reviewed changes
Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

Left some comments

app/components/UI/Ramp/common/Views/OrderDetails/OrderDetails.tsx Outdated Show resolved Hide resolved
app/components/UI/Ramp/common/Views/OrderDetails/OrderDetails.tsx
const handleMakeAnotherPurchase = useCallback(() => {
navigation.goBack();
navigation.navigate(Routes.RAMP.BUY);
}, [navigation]);
navigation.navigate(
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the UI respond the same way if you remove navigation.goBack or do both pages live on the same stack?

Copy link
Member

Choose a reason for hiding this comment

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

They are not on the same stack, the back dismisses the current order details and then the navigate opens the Buy or Sell flow.

app/components/UI/Ramp/common/Views/OrderDetails/OrderDetails.tsx
<ErrorView
description={error}
ctaOnPress={handleOnRefresh}
location="Order Details Screen"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a constants file that we can put this in?

Copy link
Member

Choose a reason for hiding this comment

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

Yes this is already a type, if you input anything different than what's defined it will error (location type mismatch). https://github.com/MetaMask/metamask-mobile/blob/feat/ramp-sell-order-details/app/components/UI/Ramp/common/types/analytics.ts#L1-L12

georgeweiler and others added 9 commits November 14, 2023 09:50
@georgeweiler
refactor(ramp): Use optional chaining for order state check
b722221
@wachunei
refactor(ramp): remove cancel button from order details
e2331b9
@georgeweiler
fix(ramp): fixes the amount received row for sell orders
35e5e5c
@georgeweiler
fix(ramp): udpates some order details fields to be the correct values…
c73ab01 
… for sell orders
@georgeweiler
chore(ramp): adds local for completed sell order details copy
b0d789f
@georgeweiler
Merge branch 'feat/off-ramp' of https://github.com/MetaMask/metamask-…
7ec79ca 
…mobile into feat/ramp-sell-order-details
@georgeweiler
Merge branch 'feat/off-ramp' of https://github.com/MetaMask/metamask-…
9f8cefd 
…mobile into feat/ramp-sell-order-details
@georgeweiler
feat(ramp): send user to the transaction confirmation screen after pr…
a251b1b 
…ovider redirect
@wachunei
refactor(ramp): remove cancel button from order send transaction
abba5fc
@georgeweiler georgeweiler force-pushed the feat/ramp-sell-order-details branch 2 times, most recently from 63a00d0 to abba5fc Compare November 16, 2023 15:34
@wachunei
Copy link
Member

@Cal-L this is ready for your re-review. I have responded to your comments. Since your last review we have:

  • Removed wrongly placed Cancel buttons from the OrderDetails and SendTransaction view (e2331b9, abba5fc)
  • Fixed some of the displaying rows in the OrderDetails view (35e5e5c, c73ab01)
  • Added a localization for the previously mentioned point (b0d789f)
  • Added an automatic redirection to the SendTransaction view when adding an order (a251b1b)
  • Added test for the redirection and analytics (59868b1)

@wachunei wachunei force-pushed the feat/ramp-sell-order-details branch from 12a7983 to 59868b1 Compare November 16, 2023 19:41
@wachunei
Copy link
Member

@Cal-L any further comments/changes will be addressed in a new PR

@sonarqubecloud SonarQubeCloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

77.1% 77.1% Coverage
0.0% 0.0% Duplication

@bkirb
Copy link
Contributor

bkirb commented Nov 16, 2023

@georgeweiler LGTM I verified the order details view ✅

@wachunei wachunei merged commit dbc7519 into feat/off-ramp Nov 16, 2023
@wachunei wachunei deleted the feat/ramp-sell-order-details branch November 16, 2023 19:55
@github-actions github-actions bot locked and limited conversation to collaborators Nov 16, 2023
@Cal-L
Copy link
Contributor

Cal-L commented Nov 16, 2023

@Cal-L any further comments/changes will be addressed in a new PR

Sounds good!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wachunei wachunei wachunei approved these changes

@Cal-L Cal-L Cal-L left review comments

Assignees
No one assigned
Labels
needs-ramp-qa Tickets that need feature QA on the ramps flows team-ramp issues related to Ramp features
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@georgeweiler @codecov-commenter @wachunei @bkirb @Cal-L