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

follow-up(PR771): quick freeze/unfreeze #802

Merged
merged 21 commits into from
Aug 16, 2024
Merged

Conversation

theborakompanioni
Copy link
Collaborator

@theborakompanioni theborakompanioni commented Jul 22, 2024

Follow-up to #771.

TBD.

Still TODO:

  • Handle frozen/unfrozen expired/non-expired fidelity bonds correctly
  • Handle bond tags correctly (i.e. not using the translated display string for identification)
  • ShowUtxos should not use ConfirmModal (instead use an own/new distinct modal component?)
  • Should ShowUtxos refactored to be external to SourceJarSelector?
  • Rename ShowUtxos -> QuickFreezeUtxoModal?
  • etc.

@theborakompanioni theborakompanioni added the refactoring Code refactoring without functional changes label Jul 22, 2024
@theborakompanioni theborakompanioni self-assigned this Jul 22, 2024
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.

Wow, the code looks way cleaner and has a precise approach! The improvements are really clear. I did notice a few UI-related aspects that might need some attention:

  1. The checkbox is too close to the border.
  2. The tags are positioned too close to the balance.
  3. The cancel button is larger than the confirm button.
  4. I also remember having a meeting with Edi where he mentioned that initially, only 5 UTXOs should be visible, and the rest should be scrollable.

I am nowhere near @editwentyone when it comes to UI, so I think it might be helpful if @editwentyone could take a look at the modal.

Lastly, just a thought: Previously, users were not allowed to unfreeze the last UTXO and proceed with the transaction, but now they can. Considering this change, I came across two conclusions:

  1. Maybe we shouldn’t restrict users from doing what they want, as they’re ultimately responsible for their actions.
  2. Most users tend to learn by tinkering around rather than reading documentation. So, in my opinion, we should restrict users from doing that.
    What do you think?

@theborakompanioni
Copy link
Collaborator Author

theborakompanioni commented Aug 13, 2024

Hey @amitx13! Appreciate the valuable feedback!

Wow, the code looks way cleaner and has a precise approach! The improvements are really clear. I did notice a few UI-related aspects that might need some attention:

1. The checkbox is too close to the border.

Changed when switching from ConfirmModal to an own modal. Did not give too much attention to UI details yet, but will definitely take care of it, once the code is "ready". Thanks!

2. The tags are positioned too close to the balance.

👍

3. The cancel button is larger than the confirm button.

Ha, how could I not notice that! Thanks 🥲

Edit: It's with all the buttons in modal footers -> will be fixed in a different PR.

4. I also remember having a meeting with Edi where he mentioned that initially, only 5 UTXOs should be visible, and the rest should be scrollable.

Yes. Good idea. I am still unsure how to make it more obvious that the list is scrollable and there are more UTXOs below..

I am nowhere near @editwentyone when it comes to UI, so I think it might be helpful if @editwentyone could take a look at the modal.

Yes, feedback from @editwentyone is highly appreciated 🙏

Lastly, just a thought: Previously, users were not allowed to unfreeze the last UTXO and proceed with the transaction, but now they can. Considering this change, I came across two conclusions:

1. Maybe we shouldn’t restrict users from doing what they want, as they’re ultimately responsible for their actions.

2. Most users tend to learn by tinkering around rather than reading documentation. So, in my opinion, we should restrict users from doing that.
   What do you think?

Like you clear thinking on this one. But yes, this is intentional, however, open for discussion. Personally, I'd go for option 1.

@theborakompanioni
Copy link
Collaborator Author

theborakompanioni commented Aug 15, 2024

Ready for review.. it's not yet in the shape I would like it to be, but it's better than before and should be good enough to build on. Especially the tag handling should be improved and refactored in a follow-up PR. But also some UI adaptions are still necessary.

@theborakompanioni theborakompanioni marked this pull request as ready for review August 15, 2024 08:10
@theborakompanioni theborakompanioni merged commit ead874a into devel Aug 16, 2024
@theborakompanioni theborakompanioni deleted the refactor/771-followup branch August 16, 2024 13:15
@0xSaksham
Copy link
Contributor

0xSaksham commented Aug 17, 2024

image

Isn't there supposed to be a tick symbol here?

Also, if you want I can fix the UI issues here.

amitx13 added a commit that referenced this pull request Aug 17, 2024
barrytra added a commit to barrytra/jam that referenced this pull request Aug 18, 2024
@theborakompanioni
Copy link
Collaborator Author

Isn't there supposed to be a tick symbol here?

Yes, you are right - this is done in a follow-up PR. Can you review and add feedback for #815?

Also, if you want I can fix the UI issues here.

Any contribution is welcome! Best would be to focus on bringing your current PRs on master but any improvement is nice to have and if you see some defect, fixing them is always appreciated.

amitx13 added a commit that referenced this pull request Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Code refactoring without functional changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants