Unify modals "close" button

trezor / trezor-suite

Trezor Suite Monorepo

https://trezor.io/trezor-suite

Other

687 stars 246 forks source link

Unify modals "close" button #5618

Open szymonlesisz opened 2 years ago

szymonlesisz commented 2 years ago

Close buttons have inconsistent layout in different modals. It would be nice to have them displayed in the same way everywhere.

Examples:

Screenshot from 2022-06-15 11-02-31

Screenshot from 2022-06-15 11-03-28

Screenshot from 2022-06-15 11-02-52

dahaca commented 1 year ago

I'd say we use the "abort" button from Image 2 in all device prompt modals which can are cancelable. So far it was only done for generic device prompts, the "follow the instruction" ones. It can now be extended to more specific ones, like the "send" one from the Image 1. The modal on Image 3 does not interact with the device so it should remain as is.

komret commented 7 months ago

@dahaca I context of the current redesign, is there anything to do, or should we close this issue?

dahaca commented 7 months ago

@komret I would say not close but restructure: the issue Szymon pointed to is that the close button is in different places, however I would argue that the AbortButton is essentially like the close button with additional context – the word "Abort". Currently it does not inherit its style from the IconButton global component, which it eventually should. And I never was a fan of the cancel decive request button located in the "device pill" but that is out of the scope of the redesign, as it's a very comprehensive issue in itself. 🙏

TLDR: we should eventially restyle the AboutButton to make it looks like the IconButton – this is scoped in. "Device pill" logic – not scoped in. If you agree, please rename the issue and add it to the milestone ✌️

komret commented 7 months ago

@dahaca

Using IconButton in AccountButton sounds reasonable. The default Modal close button looks different in the new design so this will solve the original issue of the close button looking differently in image 2 and image 3. I can create an issue for that.
This won't solve the fact that the abort button is sometimes in the pill, sometimes in the modal, which is obviously wrong. Fixing it will resolve this issue. However, we need a decision where the button should be. (See image 1 vs image 2 above).

Agreed?

dahaca commented 7 months ago

@komret solving the device indicators is not a part of the first phase of empower 🙏 But sure, polishing the about Button is very much doable!