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

Epic/redesign #779

Open
wants to merge 16 commits into
base: devel
Choose a base branch
from
Open

Conversation

barrytra
Copy link
Contributor

this PR fixes #650
initial commits focus on the design for the new FB creation modal landing page.
New design for FB

@barrytra
Copy link
Contributor Author

Hey @theborakompanioni
As of now, the basic design of new modal has been implemented.
In the new modal, it has 4 steps. out of which first 2 have been implemented.

Suggestions needed for the 3rd step :
Earlier we had few more steps namely:

  1. select UTXOs
  2. freeze UTXOs
  3. review inputs
  4. Unfreeze UTXOs
    Now what I understand is all these things are somehow to be tackled in step 3 itself.
    Also is the similar to what @amitx13 is implementing in (ui) Implement UTXO Selection Modal  #765 ?

@barrytra barrytra self-assigned this Jun 13, 2024
@barrytra barrytra added this to the SoB milestone Jun 13, 2024
@barrytra barrytra marked this pull request as draft June 15, 2024 16:41
@barrytra
Copy link
Contributor Author

barrytra commented Jun 15, 2024

Progress :

  1. modal UI have been converted to the new one as suggested. Currently Dropdowns work only by clicking next or back button and not by directly clicking on them.
  2. 1st and 2nd step have been implemented. 3rd step(select UTXOs) is the major one, but still to be touched.

Issues to be tackled as of now:

  • Improve the UI for 1st and 2nd step(mainly the first step)
  • In step 1, the date is not getting updated to the new value. More precisely when user selects a month or year in the form. It changes for a moment and the reverts back to default value. This needs to be fixed.

@barrytra barrytra requested a review from editwentyone June 15, 2024 16:56
@barrytra
Copy link
Contributor Author

barrytra commented Jul 8, 2024

Subtasks Done:

  • Got rid of step StepComonent() and simplified the code.
  • The value of lockDate now updates perfectly and does not go back to original value.
  • Now timeLockedAddress is visible at the very start of creating a new Fidelity Bond.

@barrytra
Copy link
Contributor Author

barrytra commented Jul 8, 2024

Upcoming subtasks:

  • Storing the Utxos selected by user even if the user goes back/next.
  • Make sure, on reaching the fourth step, all three selected data persist before the confirmation.
  • Implement UI of Confirmation step.

@theborakompanioni theborakompanioni changed the base branch from master to devel July 8, 2024 14:43
@barrytra barrytra marked this pull request as ready for review August 9, 2024 13:23
@barrytra barrytra requested review from 0xSaksham and amitx13 August 18, 2024 18:12
0xSaksham
0xSaksham previously approved these changes Aug 19, 2024
@editwentyone
Copy link

editwentyone commented Aug 20, 2024

@barrytra

sorry for my late review, but I finally managed it :) overall it looks and feels awesome. thanks for the hard work. I will add some issues I noticed and we will go from there:

  • the accordion boxes should do the same as "back" and "next" is doing. i.e.: I select a date and could click next, but I also want to be able to open Step 2 and thus close step 1. but I can't jump forward from step 1 to step 3 or 4. so I can go next but not skip
  • if I "select potentially less private UTXOs" the button should be red (please see here)
  • during step 3, selection of utxos, its good to show the user the total calculated amount on every selection and when the user is done, keep the info on the accordion title, please see here
  • there are some alignment issues in the accordion titles, step 1 and step 2
  • last step 4 can have a confirmation checkbox instead of the number "4" (link)
  • can we use this icon instead? link
  • if I go "back" may times, from step 4 up to step 1, I lose my selected date, that's a bug, it should keep the already selected date

i am sure not every feedback needs to be at the beginning and is something for a following PR. thank you :)

Copy link

@editwentyone editwentyone left a comment

Choose a reason for hiding this comment

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

please see my post above

@barrytra
Copy link
Contributor Author

@theborakompanioni, Now the branch is rebased with devel branch. and everything looks good to me. Ready for review 🚀

@editwentyone
Copy link

@theborakompanioni @barrytra are the fixes or changes from my previous feedback already applied?

@barrytra
Copy link
Contributor Author

@theborakompanioni @barrytra are the fixes or changes from my previous feedback already applied?

No not yet. last commit was to fix the rebase issue with devel branch

@barrytra
Copy link
Contributor Author

Some points to look at @theborakompanioni :

  • After a Fb is created, Should there be some Fidelity Bond created modal?
  • After a FB is created, Sometimes I need to reload page for that FB to be visible. Does It happen with others as well? Or what can be the solution?
  • I am not very confident if I handled the cases well where there is an error creating FB. Any ideas on how we can generate errors? Like at the very last step when finally calling "spendUtxosWithDirectSend" I am not sure how errors will look like.

@theborakompanioni
Copy link
Collaborator

theborakompanioni commented Aug 23, 2024

Hey @barrytra!

Quick feedback from my side:

  • Initially "Could not load time-locked address" is always shown when opening the modal
  • "Back" button on first step: Should be "cancel"?
  • Should "Warning: A fidelity bond with the same expiry date already exists." always be visible at the top?
  • Padding at the "Creating..." modal needs some love.
  • Warning of the immutability of the locked funds (e.g. "It will be impossible to spend time-locked funds until the fidelity bond expires. Date of expiration is in 3 months on Sun, 01 Dec 2024 00:00:00 GMT.") is not shown anywhere anymore..
  • Mining fee before the transaction is not shown anymore.
  • "Funded from Jar C" (or the selected UTXO/s) is not shown anymore.

Some points to look at @theborakompanioni :

* After a Fb is created, Should there be some  `Fidelity Bond created` modal?

Question for @editwentyone.

* After a FB is created, Sometimes I need to reload page for that FB to be visible. Does It happen with others as well? Or what can be the solution?

Not sure if it happened in the legacy code - is there a routine that can wait some amount of time for the FB to be included in the API response?

* I am not very confident if I handled the cases well where there is an error creating FB. Any ideas on how we can generate errors? Like at the very last step when finally calling "spendUtxosWithDirectSend" I am not sure how errors will look like.

You can always throw an error directly in the code or e.g. turn your network off in your browser before attempting the API request. 🙌
Also, you can write some tests to verify that behaviour: "If API requests fails, an error alert should be visible."

@editwentyone
Copy link

@barrytra no modal please, the earn page itself would state a successfully created FB, please see here:

image

https://www.figma.com/design/kfejZJFlwBywvLEnPEmJo1/JoinMarket-UI?node-id=4570-84488&t=bvzLkT9bl3UVoCGl-11

@editwentyone
Copy link

looking good, two things are still open, if possible:

  • checkmark looks weirdly misaligned
image
  • going back and forth by accordion is not working (is this coming in an another ticket/ followup pr ?)

@theborakompanioni
Copy link
Collaborator

Tasks to be done (arranged in order of priority as per my thinking):

* [ ]  Make sure, when Fb is created user can see it on the earn page everytime.

* [ ]  Show mining fee before the transaction.

* [ ]  align checkmark at the confirmation step.

* [ ]  Remove the `creating...` modal and show it on the earn page itself. (Follow-up PR )

* [ ]  Enable going back and forth with the accordions as well.(Follow-up PR )

I've prioritized the tasks based on my understanding, but I'm open to adjusting the approach based on your suggestions.

Sounds awesome. Coming along really great. 🙌

@theborakompanioni theborakompanioni marked this pull request as draft September 3, 2024 09:51
@editwentyone editwentyone modified the milestones: v0.3.1, v0.4.0 Oct 22, 2024
@editwentyone
Copy link

reviewed again, no progress right? because I see the feedback is still open from last time

@barrytra
Copy link
Contributor Author

Checkboxes 2,3 and 4 have been ticked and are ready for review.

Regarding checkbox 1: I was looking into it and came to understand that, if the creation of FB is quick, reloading happens faster than FB getting into the database. That is why it does not fetch anything new on the page but as I make the network slow, things work perfectly fine. I couldn't find an optimal way to fix this and will need your help @theborakompanioni .

checkbox 5 can be tackled in follow-up PR.

Other than that the PR is ready for review. Looking forward to your insights @editwentyone @theborakompanioni

@barrytra barrytra marked this pull request as ready for review November 10, 2024 11:07
editwentyone
editwentyone previously approved these changes Nov 15, 2024
Copy link

@editwentyone editwentyone left a comment

Choose a reason for hiding this comment

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

I like it, its definitely on the right track, thank your for your contribution:

  • I would love to see the same formatting on the earn page for the active and starting FB as it's already inside the modal under "confirmation". ( i.e.: bigger, amount is also not top right)
  • if its possible we should introduce the correct JAR graphics with the clock icon, please see the Figma designs
  • I added a mining icon and formatted it under the rest of the infos Figma
image

@theborakompanioni
Copy link
Collaborator

Checkboxes 2,3 and 4 have been ticked and are ready for review.

Regarding checkbox 1: I was looking into it and came to understand that, if the creation of FB is quick, reloading happens faster than FB getting into the database. That is why it does not fetch anything new on the page but as I make the network slow, things work perfectly fine. I couldn't find an optimal way to fix this and will need your help @theborakompanioni .

checkbox 5 can be tackled in follow-up PR.

Other than that the PR is ready for review. Looking forward to your insights @editwentyone @theborakompanioni

Hey @barrytra!

I will look into it in the coming days and make some changes trying to tick all the missing checkboxes, if that is in your interest as well 🙏 Thanks for the amazing job 💪

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.

ui: redesign fidelity bond creation flow
4 participants