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: Implement confirm layout #13331

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

OGPoyraz
Copy link
Member

@OGPoyraz OGPoyraz commented Feb 4, 2025

Description

This PR adds flat confirmation layout in re-designed confirmations.

Related issues

Fixes: Task is currently in draft

Manual testing steps

N/A

Screenshots/Recordings

Before

After

Pre-merge author checklist

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.

@OGPoyraz OGPoyraz requested a review from a team as a code owner February 4, 2025 10:53
@metamaskbot metamaskbot added the team-confirmations Push issues to confirmations team label Feb 4, 2025
@OGPoyraz OGPoyraz requested a review from jpuri February 4, 2025 10:53
@OGPoyraz OGPoyraz added No QA Needed Apply this label when your PR does not need any QA effort. No E2E Smoke Needed If the PR does not need E2E smoke test run labels Feb 4, 2025
const isFlatConfirmation = FLAT_CONFIRMATIONS.includes(
approvalRequest?.type as TransactionType,
);
const ContentDisplay = isFlatConfirmation ? ScrollView : View;
Copy link
Contributor

Choose a reason for hiding this comment

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

ScrollView may still be useful if content overflow, if there is no overflow - it will anyways behave as normal view.

Copy link
Contributor

@jpuri jpuri Feb 4, 2025

Choose a reason for hiding this comment

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

Ideally this component should not know about modal / fullscreen layout. If possible differences in layout can be moved a level up in <ConfirmaionLayout> component.

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly, I was also thinking to capsulate this logic in ConfirmaionLayout. However if we want to stick up Footer to the bottom (for flat confirmations), I think this is the only viable solution. I am happy to apply any suggestions if you have on this.

const { approvalRequest } = useApprovalRequest();
const isFlatConfirmation = FLAT_CONFIRMATIONS.includes(
approvalRequest?.type as TransactionType,
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than using an array like this here can we make it dynamic, may be we check if confirmation is not initiated by DAPP, that ways we will not need not maintain it for new confirmation that we add.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's definitely something we can do. But my understanding is that flat confirmation support is not tied to origin. Maybe we could visit this in the upcoming implementations?

</View>
<Footer />
<View style={styles.container}>
{children}
</View>
</BottomModal>
);
Copy link
Contributor

@jpuri jpuri Feb 4, 2025

Choose a reason for hiding this comment

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

Rather than including <ConfirmationLayout in <Confirm can we rather include <Confirm> in place of {children} here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, done

backgroundColor: theme.colors.background.alternative,
justifyContent: 'space-between',
paddingHorizontal: 16,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure is fixed position there in react native ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately not
Screenshot 2025-02-04 at 14 10 28

Copy link

sonarqubecloud bot commented Feb 5, 2025

Quality Gate Failed Quality Gate failed

Failed conditions
B Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No E2E Smoke Needed If the PR does not need E2E smoke test run No QA Needed Apply this label when your PR does not need any QA effort. team-confirmations Push issues to confirmations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants