-
Notifications
You must be signed in to change notification settings - Fork 557
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
Create an example Snap for Dynamic Permissions #2198
Comments
13 tasks
david0xd
added a commit
to MetaMask/metamask-extension
that referenced
this issue
Apr 24, 2024
## **Description** This PR introduces some major code refactoring and UI updates to **_Connect_** and **_Permission Approval_** screens. #### Motivation There are several blockers encountered while trying to introduce (integrate) new features provided by Snaps Core Platform such as Dynamic Permissions. The major reason for a blocker is the state of the current implementation of the _Permission Approval_ component. More specifically, the large difference and inconsistencies between the UI components and overall UI structure used for the native and Snaps flows related to the permission approval features are making it close to impossible to maintain and extend with the new functionalities. Building even more fragmented conditional UI in order to introduce new features, as part of the same flows and features, would dramatically increase code complexity and maintenance demands, while resulting in poor UX/UI in the final product. Hence, after multiple discussions inside the teams and between the different teams, it is determined that the best approach would be complete UI refactoring and consolidation of the user interface between native and Snaps flows. This PR is the first major step made in order to achieve the proposed results and unblock the new features waiting for integration. #### What is done in this PR - Updated UI for Account Connection and Permission Approval screens. - Refactoring of the Connection and Permission Approval components. - Refactoring of the other components related to Connect and Permission pages. - Replaced old way of UI styling with the new one aligned with Design System approach. This includes replacement of all deprecated Design System components with the current. - Removed redundant SCSS code. UI styling is now mostly handled by Design System components. - Updated some parts of the Storybook for the related components. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/23557?quickstart=1) ## **Related issues** Fixes: #23316 Design: https://www.figma.com/file/jr7XsFvcPCnf4HOMvNWFfn/Permission-System?node-id=1%3A2&mode=dev (see facelift section). Blocks: MetaMask/snaps#1821 Blocks: MetaMask/snaps#2088 Blocks: MetaMask/snaps#2198 ## **Manual testing steps** 1. Go to [E2E Test Dapp](https://metamask.github.io/test-dapp/) or [Test Snaps](https://metamask.github.io/snaps/test-snaps/latest/) website. 2. Try connecting the accounts to the Test Dapp. Try approving permission requested. Check the UI. 3. Try installing a Snap and check all the related UI starting from a connection to the website. 4. Try installing Ethereum Provider Snap and verify the UI for the complete flow including triggering option for getting Ethereum accounts. 5. Try using Multi Install Snap feature provided by the Test Snaps web application. 6. Check any other website connection or permission approval related flows and features and make sure that there are no regressions. 7. Verify that there are no regressions in all the UX/UI related to the all features mentioned above. Also, verify the functionality of the related features. ## **Screenshots/Recordings** ### **Before** ![Screenshot 2024-03-28 at 16 05 48](https://github.com/MetaMask/metamask-extension/assets/13301024/568fc097-f2f2-4516-a921-a6596d33afca) <img width="1018" alt="Screenshot 2024-03-29 at 15 35 51" src="https://github.com/MetaMask/metamask-extension/assets/13301024/b06fa630-3912-445a-80e8-00f85a0ce301"> <img width="1462" alt="Screenshot 2024-03-29 at 15 37 26" src="https://github.com/MetaMask/metamask-extension/assets/13301024/20b4cea6-9258-482a-9eb4-38de2234f15a"> <img width="927" alt="Screenshot 2024-03-29 at 15 39 52" src="https://github.com/MetaMask/metamask-extension/assets/13301024/c7643215-78cc-43b8-8c89-5ae504e05bb2"> ### **After** ![Screenshot 2024-04-22 at 18 40 35](https://github.com/MetaMask/metamask-extension/assets/13301024/b2c903ca-0a7a-47df-ac56-f9d9b6587245) ![Screenshot 2024-04-22 at 18 41 08](https://github.com/MetaMask/metamask-extension/assets/13301024/4ad1211d-7de4-43b8-82c1-e4e5c3e23aa9) ![Screenshot 2024-04-22 at 18 41 50](https://github.com/MetaMask/metamask-extension/assets/13301024/b48b04c3-36ec-4a15-a56b-1e3dd7e466ef) ![Screenshot 2024-04-22 at 18 42 35](https://github.com/MetaMask/metamask-extension/assets/13301024/a6fd4e7f-6dd8-4482-bc72-b73f33333b4b) ![Screenshot 2024-03-29 at 15 48 17](https://github.com/MetaMask/metamask-extension/assets/13301024/ac0d3d0c-9ade-484c-b2e2-35857d47c588) ![Screenshot 2024-04-17 at 16 28 03](https://github.com/MetaMask/metamask-extension/assets/13301024/2a2bd11d-238b-488c-994e-d531a0bc281a) ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've clearly explained what problem this PR is solving and how it is solved. - [x] I've linked related issues - [x] I've included manual testing steps - [x] I've included screenshots/recordings if applicable - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. - [x] 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Create an example Snap which demonstrates how Dynamic Permissions work.
The text was updated successfully, but these errors were encountered: