Skip to content
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

modal instead of dropdown for new FB creation form #762

Merged
merged 2 commits into from
May 22, 2024

Conversation

barrytra
Copy link
Contributor

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.

@barrytra barrytra self-assigned this May 22, 2024
@theborakompanioni
Copy link
Collaborator

@barrytra Nice!

Works exactly as expected. Great work! 🚀

  • UI might need small adaptions to have the header consistent with other modals (renew bond, fee settings, seed modal, etc.):

📸

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! 🙏

  • I think the custom CSS styles are also not needed! 💪

  • Can we find a better name for variable isExpanded? e.g. showCreateFidelityBondModal

  • The sprite does not need to change from "plus" to "caret-up" anymore, when isExpanded is true

What do you think?

@barrytra
Copy link
Contributor Author

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
Copy link
Collaborator

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 💪

Copy link
Contributor

@amitx13 amitx13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup!  It's working fine. Good job, @barrytra

@barrytra barrytra merged commit 056c9d4 into joinmarket-webui:master May 22, 2024
3 checks passed
@barrytra barrytra deleted the modal-not-dropdown branch May 22, 2024 15:57
@theborakompanioni
Copy link
Collaborator

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
Copy link
Contributor Author

barrytra commented May 29, 2024

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
Copy link
Contributor Author

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

@theborakompanioni
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modal instead of dropdown for new fidelity Bond creation form
3 participants