jam icon indicating copy to clipboard operation
jam copied to clipboard

Epic/redesign

Open bhaveshg16 opened this issue 1 year ago • 17 comments

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

bhaveshg16 avatar Jun 11 '24 08:06 bhaveshg16

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 https://github.com/joinmarket-webui/jam/issues/765 ?

bhaveshg16 avatar Jun 13 '24 08:06 bhaveshg16

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:

  • [x] Improve the UI for 1st and 2nd step(mainly the first step)
  • [x] 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.

bhaveshg16 avatar Jun 15 '24 16:06 bhaveshg16

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.

bhaveshg16 avatar Jul 08 '24 09:07 bhaveshg16

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.

bhaveshg16 avatar Jul 08 '24 09:07 bhaveshg16

@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 :)

editwentyone avatar Aug 20 '24 11:08 editwentyone

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

bhaveshg16 avatar Aug 20 '24 15:08 bhaveshg16

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

editwentyone avatar Aug 21 '24 07:08 editwentyone

@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

bhaveshg16 avatar Aug 21 '24 07:08 bhaveshg16

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.

bhaveshg16 avatar Aug 23 '24 06:08 bhaveshg16

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. :raised_hands: Also, you can write some tests to verify that behaviour: "If API requests fails, an error alert should be visible."

theborakompanioni avatar Aug 23 '24 18:08 theborakompanioni

@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 avatar Aug 25 '24 21:08 editwentyone

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 ?)

editwentyone avatar Aug 28 '24 13:08 editwentyone

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.

bhaveshg16 avatar Aug 29 '24 11:08 bhaveshg16

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. :raised_hands:

theborakompanioni avatar Aug 29 '24 12:08 theborakompanioni

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

editwentyone avatar Oct 31 '24 11:10 editwentyone

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

bhaveshg16 avatar Nov 10 '24 11:11 bhaveshg16

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 :pray: Thanks for the amazing job :muscle:

theborakompanioni avatar Nov 16 '24 18:11 theborakompanioni