Conversation
CLA Assistant Lite All Contributors have signed the CLA. |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
Pull Request Test Coverage Report for Build 2154519931
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💪
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good Usame! Left some comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the comments 👍
The colors, fonts, spacing, elements, etc looks fine. If the length of the menu is not considered an issue then I'd pass the ticket to QA done. |
Confirmed that the list of safes is for another ticket, so out of the scope of this one. Regarding the length of the menu, it was decided that we will remove the "Read Only" label and make the "New Transaction" button to change the text to "Read Only" when the conditions are met (Not connected, not owner of the safe) |
It looks good to me. The button shows Read Only when the user is not connected or is not owner, and reverts to New tx when the conditions are met. |
What it solves
Resolves #3504
How this PR fixes it
How to test it
Screenshots