-
Notifications
You must be signed in to change notification settings - Fork 3
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
Stax #1
Stax #1
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed this massive PR and verified that everything works as expected. @dj8yfo has also checked that it works on Nano devices.
@agrojean-ledger Please, review and merge it
@PolyProgrammist @frol are you sure you need The generic review should always be used as a last resort. Maybe the choice use case would allow you to generate the screens you need ? It is now available in the latest version of the Rust SDK : 1.13.0 You would be able to add screens like that for instance : The API allows to add (or not) status text when the user clicks on one of the buttons. |
@agrojean-ledger Yes, I am absolutely sure. We use these review screens independently and on the fly while the data is getting streamed on the fly and we don't have the memory to load all the choices upfront. Thank you for the review, but for god's sake, please, stop doing this "and one more thing". Please, publish this app finally. |
@frol @PolyProgrammist I suggest you look at the |
What is exactly wrong with our implementation from the user perspective that you suggest us to go into refactoring again? |
It was just a suggestion. Anyway, I don't understand your code enough to know if you could replace the generic reviews by choice screens. Bump the version of the package and we'll be done with. I will merge your PR but the app will only be deployed when it is requested by our product team. |
fdbdb13
to
8a5fc6c
Compare
@frol @PolyProgrammist The CI is KO, you need to fix it before we can merge. |
@agrojean-ledger thanks for the review. Could you please rerun CI? |
@agrojean-ledger Please, approve CI once again |
@frol @PolyProgrammist There's still one job that fails... you're almost there. |
Checklist
develop