chore: disable protect modal in development

MetaMask / metamask-mobile

Mobile web browser providing access to websites that use the Ethereum blockchain

https://metamask.io

Other

2.03k stars 1.06k forks source link

chore: disable protect modal in development #10110

Open dbrans opened 3 days ago

dbrans commented 3 days ago

Description

This PR disables the "Protect your wallet" blocking modal when NODE_ENV is 'development'

Related issues

Fixes:

Manual testing steps

Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

[ ] I’ve followed MetaMask Contributor Docs and MetaMask Mobile Coding Standards.
[ ] I've completed the PR template to the best of my ability
[ ] 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.

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.

github-actions[bot] commented 3 days ago

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

tommasini commented 3 days ago

I don't know if it's a good idea to hide this always when it's development, we identified recently bug on that modal, if we apply this we will always miss if any bug is present during development I think we could create a variable for this but one specific Open to more thoughts

sonarcloud[bot] commented 3 days ago

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
66.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

dbrans commented 3 days ago

I don't know if it's a good idea to hide this always when it's development, we identified recently bug on that modal, if we apply this we will always miss if any bug is present during development I think we could create a variable for this but one specific Open to more thoughts

@tommasini I think this could be a good improvement: default the modal to off during development. Developers who need to debug it can just comment out a line as they do now. What do you think?

tommasini commented 3 days ago

I think this could be a good improvement: default the modal to off during development. Developers who need to debug it can just comment out a line as they do now. What do you think?

If I understood correctly you are suggesting that the default behaviour is to hide the protect modal to funded not backed up accounts. I am not sure just because we don't have anything educating the engineers that this was a feature, some of the new engineers probably don't know it, and if we turn this on by default for not showing the protect modal, I'm afraid they can only discover when they create a bug on it! Sorry, I am just trying to think how this could go wrong, If more team members (QA team included) are okay with this, I'm OK with moving this further!