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

Stax #1

Merged
merged 42 commits into from
Jul 31, 2024
Merged

Stax #1

merged 42 commits into from
Jul 31, 2024

Conversation

PolyProgrammist
Copy link
Contributor

@PolyProgrammist PolyProgrammist commented Jul 24, 2024

Checklist

  • App update process has been followed
  • Target branch is develop
  • Application version has been bumped

Copy link
Contributor

@frol frol left a 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

Cargo.toml Outdated Show resolved Hide resolved
@agrojean-ledger
Copy link
Contributor

agrojean-ledger commented Jul 26, 2024

@PolyProgrammist @frol are you sure you need NbglGenericReview where you are using it ?

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 :

TestUseCaseChoice

The API allows to add (or not) status text when the user clicks on one of the buttons.

@frol
Copy link
Contributor

frol commented Jul 26, 2024

are you sure you need NbglGenericReview where you are using it ?

@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.

@agrojean-ledger
Copy link
Contributor

are you sure you need NbglGenericReview where you are using it ?

@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 NbglChoice that was just added to the SDK, I think it would be more fitting where you try to use it.

@frol
Copy link
Contributor

frol commented Jul 26, 2024

I suggest you look at the NbglChoice that was just added to the SDK, I think it would be more fitting where you try to use it.

What is exactly wrong with our implementation from the user perspective that you suggest us to go into refactoring again?

@agrojean-ledger
Copy link
Contributor

agrojean-ledger commented Jul 26, 2024

I suggest you look at the NbglChoice that was just added to the SDK, I think it would be more fitting where you try to use it.

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.

@agrojean-ledger
Copy link
Contributor

@frol @PolyProgrammist The CI is KO, you need to fix it before we can merge.

@PolyProgrammist
Copy link
Contributor Author

@agrojean-ledger thanks for the review. Could you please rerun CI?

@frol
Copy link
Contributor

frol commented Jul 30, 2024

@agrojean-ledger Please, approve CI once again

@agrojean-ledger
Copy link
Contributor

@frol @PolyProgrammist There's still one job that fails... you're almost there.

@agrojean-ledger agrojean-ledger merged commit 229a2b5 into LedgerHQ:develop Jul 31, 2024
32 checks passed
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.

3 participants