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

Replace old send page with new new one - Closes #1743 #1817

Merged
merged 42 commits into from Mar 22, 2019

Conversation

osvaldovega
Copy link
Contributor

What issue have I solved?

-- #1743

How have I implemented/fixed it?

In this PR the routes file has been update to redirect to the folder that contains the new version (design) of the send page.

Then update the E2E tests that are related to the send page, all the tests for this page were update

How has this been tested?

Open the app the go to wallet and do a click on the send button, this will display the new version of send and then user can go through the process.
You can send and LSK by typing address or selecting one if there is a bookmark already or using the HW ledger.

Review checklist

@osvaldovega osvaldovega self-assigned this Mar 12, 2019
@slaweet
Copy link
Contributor

slaweet commented Mar 13, 2019

I quickly tried it out and found these two things to fix:

  • Top bar "My wallet" icon shold be also blue
  • "Edit transaction" doesn't keep the values. Clicking it will go to the previous step with all inputs empty.
    Screen Shot 2019-03-13 at 10 20 44

@osvaldovega osvaldovega changed the base branch from 1713-implement-send-submitted-view to 1.14.0 March 14, 2019 09:13
@osvaldovega osvaldovega requested a review from massao March 14, 2019 09:32
Copy link
Contributor

@massao massao left a comment

Choose a reason for hiding this comment

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

Just a few comments

src/components/sendV2/summary/summary.js Show resolved Hide resolved
src/components/sendV2/summary/summary.js Outdated Show resolved Hide resolved
src/components/sendV2/summary/summary.test.js Outdated Show resolved Hide resolved
test/cypress/utils/enterSecondPassphrase.js Outdated Show resolved Hide resolved
@osvaldovega
Copy link
Contributor Author

@slaweet @Efefefef I just update the PR based on the last comments right now everything is working and have the texts based on the same.
Please let me know if it is ok like this.

Osvaldo Vega added 2 commits March 19, 2019 17:32
…com:LiskHQ/lisk-hub into 1743-replace-old-send-page-with-new-new-one
Copy link
Contributor

@Efefefef Efefefef left a comment

Choose a reason for hiding this comment

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

Please consider my comment above, it contains actual bugs

🐛 Byte counter is broken. try entering 🇩🇪
🐛 Tooltip's wrong tip's direction

image

🐛 Typo
image

@Efefefef
Copy link
Contributor

🐛
Should be bookmarked account
image

@Efefefef
Copy link
Contributor

Efefefef commented Mar 21, 2019

Uncaught (in promise) Error: Status 404 : Delegate not found is still not fixed

Please check the comment above. Why you put bookmark account, not bookmarked account ?

@osvaldovega
Copy link
Contributor Author

@Efefefef I update the bookmark labels with the correct text, I just checked with @slaweet.
About the error message in the console, that is a problem with the lisk core, I need to create a ticket with them it is not related to us because the problem is randomly, thats why I didn't saw it before submit the PR the last time and then you saw it now. it is not all the times that happen

@Efefefef
Copy link
Contributor

Efefefef commented Mar 21, 2019

🐛 Create First Transaction from Initialize Lisk ID widget doesn't fill in the fields
@slaweet Should we show Account bookmarked button on Confirmation page if that was an already bookmarked account?
image

@osvaldovega
Copy link
Contributor Author

@Efefefef In the meetings that we have for check this designs Daria already mentioned that only is displayed when the account it is not part of the bookmarks list, so if account is already in the bookmark list we don't show the button.
cc @slaweet

@slaweet
Copy link
Contributor

slaweet commented Mar 21, 2019

@osvaldovega @Efefefef Account bookmarked button should be only displayed in the flow the user bookmarked the account. It should not be displayed the next time the user sends to that address.

@Efefefef
Copy link
Contributor

Ok guys then there is a bug
🐛 If you click Send to the acc from account's page then you'll see the bookmarked dialog shown

@osvaldovega
Copy link
Contributor Author

@Efefefef can you please check all the functionality related the send component and confirm that everything else is working as should be, in case something else it is not working properly can you please put it in just one update, then I don't need to to fix just one bug and after that you check again the code and find another one.

In case this is the last bug, can you please confirm this is the last thing to fix then I can proceed to fix it?

Efefefef
Efefefef previously approved these changes Mar 22, 2019
@Efefefef
Copy link
Contributor

👍 Good to go!

@Efefefef
Copy link
Contributor

🐛 Man you accidentally removed the link that prepopulates fields

@osvaldovega osvaldovega merged commit a630a92 into 1.14.0 Mar 22, 2019
@osvaldovega osvaldovega deleted the 1743-replace-old-send-page-with-new-new-one branch March 22, 2019 14:30
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.

None yet

4 participants