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.

Sign up for GitHub

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

Jump to bottom

Onboarding #56

Merged
merged 16 commits into from
Dec 13, 2020
Merged

Onboarding #56

merged 16 commits into from
Dec 13, 2020

Conversation

aspic
Copy link
Collaborator

@aspic aspic commented Dec 12, 2020

Added a proper flow for onboarding. Placeholder texts and values so far! Let me know what you think.

@aspic aspic requested a review from Citrullin December 12, 2020 22:45
@Citrullin
Copy link
Contributor

  1. Okay, I upgraded the wallet and it gives me a wrong pin error, while the pin is definitely right. 4 times zero. This is worth discovering in the future. See, if this happens again.
  2. Once I generated a new wallet, I am not able to scroll down to see the actual seed. this worked before. Maybe related to Fix scrolling to update #44
  3. I've backed up my seed on the right side is too long.
  4. Clicking on I've backed up my seed gets you into the main menu.

@aspic aspic marked this pull request as draft December 13, 2020 13:23
@aspic
Copy link
Collaborator Author

aspic commented Dec 13, 2020

Did not think of how to "upgrade" an existing wallet, this is something we have to be careful with in the future (once the implementation stabilizes it won't be much of a problem). Fixed some of the apparently broken things with the PR, left a few things as is so it does not grow even more:

@aspic aspic marked this pull request as ready for review December 13, 2020 13:52
@Citrullin
Copy link
Contributor

  • "Create New" needs to be shorter. It looks like there is only one option. It probably has to be just "New".
  • Scrolling still doesn't work.
  • There is a horizontal scrolling bar in the wallet. Wasn't there before. You cannot scroll it. So, just a minor issue.
  • When adding account, it probably makes sense to ask for a name there as well. But we can make a separate issue for that.
  • When receiving funds and updating the wallet with the update button, the amount in the top doesn't get updates, but the transactions are in the transaction history.
  • When I am in the setting of changing the account name, I am not able to go back. Guess this is related to Prevent "backspace" in input fields to pop history #40

We can also make separated issues for that and just merge it. I take a look into the code then.

Citrullin
Citrullin reviewed Dec 13, 2020
View reviewed changes
public/locales/en-US.properties
@@ -77,3 +70,31 @@ unlocking-wallet = Unlocks..
adding-account = Adds new account..
loading-refresh = Loads transactions..
sending-funds = Sending funds..

# Onboard
onboard-button-back = Back
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to use a prefix for everything? Like onboard__ and create__ or something similar?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it can be smart so that we know the context of the text. However, when we finish with most of the text it's probably easier to see what are the common things, and what we might need context for (if any).

Citrullin
Citrullin reviewed Dec 13, 2020
View reviewed changes
src/App.svelte Outdated Show resolved Hide resolved
Citrullin
Citrullin reviewed Dec 13, 2020
View reviewed changes
src/machinery/eventListener.ts
@@ -22,18 +26,20 @@ const elementSelector = (selectedElement) => {
let navigation = new Navigation([], elementSelector);

loadedComponentStore.subscribe((value) => {
if (value.elements.length > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you remove it? If the elements are empty, it shouldn't create a navigation and attach keydown listener to it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because we need to initialize this to set/unset selection and navigation on view change. So even though elements are an empty array, we need to know that in order to do right thing in for instance "Enter" case.

Citrullin
Citrullin reviewed Dec 13, 2020
View reviewed changes
src/machinery/wallet.ts
};
export function generateWallet(): Promise<WalletResult> {
return new Promise((resolve) => {
setTimeout(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this setTimeout for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To be able to delay the function, so that we can show the loading screen. There might be a more ideomatic way to do this though..

This was referenced Dec 13, 2020
Open
Closed
@aspic
Copy link
Collaborator Author

aspic commented Dec 13, 2020

Fixed the inconsistencies, and added a few of the unknown issues as new issues. Thanks for testing!

@aspic aspic merged commit 6d05374 into master Dec 13, 2020
@aspic aspic deleted the onboarding branch December 13, 2020 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Citrullin Citrullin Citrullin left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aspic @Citrullin