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

Refactoring Merchant Mode #432

Merged
merged 51 commits into from Jan 22, 2021
Merged

Refactoring Merchant Mode #432

merged 51 commits into from Jan 22, 2021

Conversation

n13
Copy link
Member

@n13 n13 commented Jan 14, 2021

πŸ—ƒ Github Issue Or Explanation for this PR. (What is it supposed to do and Why is needed)

Issue - Merchant mode had a strange UX and many bugs

Fix - redid merchant mode, split into multiple different screens as a good enough beta version.

Merged the ability to change the list order of merchant goods from Igor's PR

  • Add products in SEEDS and FIAT
  • Edit products in SEEDS and FIAT - even if they were entered in a different fiat currency
  • Reorder items (from Igor)
  • Reflect new order in the main selection screen
  • Enter a custom amount
  • Verify a list of items
  • New edit button in upper right corner
  • Showing totals

βœ… Checklist

  • I have tested all my changes.

πŸ•΅οΈβ€β™‚οΈ Notes for Code Reviewer

Never mind so much about the code, just run through some basic UX testing if you wish - it's too much code to review

Many, many things were broken before so this is definitely better than what we had.

πŸ™ˆ Screenshots

Screen.480p.mov

πŸ‘―β€β™€οΈ Paired with

me myself and I

@gguij004
Copy link
Collaborator

Hi Nick, I was testing your recent commits and I found some bugs when Adding and removing products to be displayed. if an user try to delete a product it goes to negative instead of disappearing. Also when they click on a product to be added it wont take you to the receive screen and product is also added with a quantity of 0.

@gguijarro-c-chwy
Copy link
Collaborator

I am not sure this receive + products feature is ready for users.

The UI is so ugly it makes me throw up and the are so many bugs that i dont even know where to start.

I think we should have jason design this feature and we should put some work on it.

@n13
Copy link
Member Author

n13 commented Jan 15, 2021

Will try to fix this up today

Entry does seem to have a lot of bugs

@n13
Copy link
Member Author

n13 commented Jan 15, 2021 via email

Copy link
Collaborator

@gguijarro-c-chwy gguijarro-c-chwy left a comment

Choose a reason for hiding this comment

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

Bugs

@gguij004
Copy link
Collaborator

Maybe "edit Products " instead of "edit" by itself?

image

@gguij004
Copy link
Collaborator

I think product should have a label like before? like "Price" not sure if we are taking the label out.

image

we dont need label?

@gguij004
Copy link
Collaborator

Receipt still have overflow error for small devices

image

Copy link
Collaborator

@gguij004 gguij004 left a comment

Choose a reason for hiding this comment

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

Hi Nick, nice job fixing all the bugs, in the UI testing side only missing some optional request and the small device overflow on receipt.

Copy link
Collaborator

@gguij004 gguij004 left a comment

Choose a reason for hiding this comment

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

weird, this did not happen on previous commits, please double check validation

image

@gguij004
Copy link
Collaborator

We allow users to create negative products.

not sure what will happen on this example.

Maybe we can let then create negative products to use as discounts,
but if the total in receipt is negative they should not be able to click next, Please advise. Thanks

image

@gguijarro-c-chwy
Copy link
Collaborator

We allow users to create negative products.

not sure what will happen on this example.

Maybe we can let then create negative products to use as discounts,
but if the total in receipt is negative they should not be able to click next, Please advise. Thanks

image

Yeah, this is no bueno.

@n13
Copy link
Member Author

n13 commented Jan 21, 2021

Negative is no good yes...

@n13
Copy link
Member Author

n13 commented Jan 21, 2021

Maybe "edit Products " instead of "edit" by itself?

image

No... edit is standard. Also would be too long for a button.

@n13
Copy link
Member Author

n13 commented Jan 21, 2021

I think product should have a label like before? like "Price" not sure if we are taking the label out.
imagew

we dont need label?

Added hint text ... please consider this is an alpha feature we just want to enable people to start using it

There will be a redesign

image

@n13
Copy link
Member Author

n13 commented Jan 21, 2021

weird, this did not happen on previous commits, please double check validation

image

Fixed

@n13
Copy link
Member Author

n13 commented Jan 21, 2021

Small devices - changed layout

We can either remove the image, or make the text smaller

image

@n13
Copy link
Member Author

n13 commented Jan 21, 2021

Fixed negative entry - not allowed

This happened because I refactored the amount field to be dual currency, to only have 1 implementation, and to be used everywhere, in transfer and also in entry here.

So overall - my bad for not testing it properly, but not wasted time to get this right.

Thanks for finding all these!

image

Copy link
Collaborator

@gguij004 gguij004 left a comment

Choose a reason for hiding this comment

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

bug on validation

image

@gguij004
Copy link
Collaborator

still broken for small device. not sure if we should looking into it since we are going to redesign feature.

image

@n13
Copy link
Member Author

n13 commented Jan 22, 2021

bug on validation

image

Fixed

@n13
Copy link
Member Author

n13 commented Jan 22, 2021

still broken for small device. not sure if we should looking into it since we are going to redesign feature.

image

Ok then, no pix on small devices ;)

Fixed

image

Copy link
Collaborator

@gguij004 gguij004 left a comment

Choose a reason for hiding this comment

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

Test all commits again, only found this bug.
I think it should be ready to merge from the UI side after this is fix.

image

@n13
Copy link
Member Author

n13 commented Jan 22, 2021

Ummm whoa that's not a scroll view! Well - thanks!

@n13 n13 merged commit cbe57bd into master Jan 22, 2021
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