joinmarket-webui / jam

Your sats. Your privacy. Your profit.
https://jamapp.org
MIT License
236 stars 48 forks source link

modal instead of dropdown for new FB creation form #762

Closed barrytra closed 1 month ago

barrytra commented 1 month ago

this PR fixes #760 When user clicks "configure fidelity bond" modal appears like this: Screenshot from 2024-05-22 12-17-39 Whole flow to create new bond works fine on my setup. Open to any kind of suggestions.

PS: Cross Button at top-right functions same as cancel button. I've kept it as it will be useful for later UI design.

theborakompanioni commented 1 month ago

@barrytra Nice!

Works exactly as expected. Great work! :rocket:

:camera_flash:

i.e. by using:

<rb.Modal show={isExpanded} animation={true} backdrop="static" centered={true} onHide={() => setIsExpanded(false)}>
<rb.Modal.Header closeButton>
  <rb.Modal.Title>{t('earn.fidelity_bond.create_fidelity_bond.title')}</rb.Modal.Title>
</rb.Modal.Header>
<rb.Modal.Body>
[...]
</rb.Modal.Body>
</rb.Modal>

See file SpendFidelityBondModal.tsx for examples! :pray:

What do you think?

barrytra commented 1 month ago

Yes.. i agree to suggestions and they have been implemented. My only concern to use customised CSS was because even in light mode the header appears to be dark. Screenshot from 2024-05-22 15-01-35 But its okay if other modals have the same flow. please have a check

theborakompanioni commented 1 month ago

Yes.. i agree to suggestions and they have been implemented. My only concern to use customised CSS was because even in light mode the header appears to be dark. But its okay if other modals have the same flow. please have a check

Good thinking! Imho, it is okay and better to stay consistent. Header color in light mode can be changed in a different PR, if needed!

tACK. Thanks @barrytra :muscle:

theborakompanioni commented 1 month ago

It seems, with this change, the alert is displayed outside of the modal in case something is wrong.

Would you be able to take another look @barrytra ?

barrytra commented 1 month ago

Ahh!! I get it. It was actually placed initially according to dropdown. This should be fixed. What do you think shall it be shown inside the modal itself?

barrytra commented 1 month ago

Also can you suggest a way to produce this error??

theborakompanioni commented 1 month ago

Ahh!! I get it. It was actually placed initially according to dropdown. This should be fixed. What do you think shall it be shown inside the modal itself?

That would probably be best, yes. Otherwise it might not be visible to the user.

Also can you suggest a way to produce this error??

You can temporarily adapt the request code and throw an error here.