8000 [LIVE-1317] "something else" choice at repair modal for Nano S by alexandremgo · Pull Request #4926 · LedgerHQ/ledger-live-desktop · GitHub
[go: up one dir, main page]
Skip to content
This repository was archived by the owner on Jun 16, 2022. It is now read-only.

[LIVE-1317] "something else" choice at repair modal for Nano S #4926

Merged
merged 4 commits into from
Apr 29, 2022
Merged

[LIVE-1317] "something else" choice at repair modal for Nano S #4926

merged 4 commits into from
Apr 29, 2022

Conversation

alexandremgo
Copy link
Contributor
@alexandremgo alexandremgo commented Apr 14, 2022 •
edited
Loading

🦒 Context (issues, jira)

LIVE-1317

The "something else" choice redirects to the end of the usb-troubleshooting flow

Add to adapt a bit compared to the given design (from figma): the Nano S repair modal is old and has not been updated to the new troubleshooting screens.

💻 Description / Demo (image or video)

Video: new feature
bugfix-live-1317-2022-04-14_09.33.25.mp4
Video: the other repair modal screens are not broken
bugfix-live-1317-no-break-2022-04-14_09.51.31.mp4

🖤 Expectations to reach

  • on QA: at least one of these two checkboxes must be checked:
    • a specific test planned is defined on Jira
    • this PR is covered by automatic UI test
  • on delivery: at least one of these two checkboxes must be checked:
    • Option 1: no impact: The changes of this PR have ZERO impact on the userland (invisible for users)
    • Option 2: atomic delivery: the changes is atomic and complete (no partial delivery)

@alexandremgo alexandremgo requested a review from a team as a code owner April 14, 2022 07:44
@github-actions
Copy link

Thanks for your contribution.
To be able to merge in develop branch, you need to:

  • pass the CI
  • if needed, run /generate-screenshots
  • have a dev review
  • have a QA review
  • if needed, /upgrade-llc

Why /generate-screenshots ?

If your PR contains UI related changes,
it might be necessary to regenerate screenshots.

Why /upgrade-llc ?

If your PR requires an update to the ledger-live-common library,
once the PR is merged on develop on ledger-live-common side,
you need to run /upgrade-llc to switch back to ledger-live-common@develop here before merging.

@github-actions
Copy link
github-actions bot commented Apr 14, 2022 •
edited
Loading

Coverage report

St.❔
 Category Percentage Covered / Total
🔴 Statements 8.74% 9/103
🔴 Branches 0% 0/19
🔴 Functions 2.94% 1/34
🔴 Lines 8.33% 8/96

Test suite run success

1 tests passing in 1 suite.

Report generated by 🧪jest coverage report action from 3ed7e50

thomasrogerlux
thomasrogerlux approved these changes Apr 14, 2022
View reviewed changes
Copy link
Contributor
@thomasrogerlux thomasrogerlux left a comment

Choose a reason for hiding this comment

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

I don't understand why this is specific to Nano S but code looks good

juan-cortes
juan-cortes approved these changes Apr 25, 2022
View reviewed changes
Copy link
Contributor
@juan-cortes juan-cortes left a comment

Choose a reason for hiding this comment

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

lgtm

juan-cortes
juan-cortes approved these changes Apr 27, 2022
View reviewed changes
Copy link
Contributor
@juan-cortes juan-cortes left a comment

Choose a reason for hiding this comment

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

Just nitpicking

src/renderer/components/RepairDeviceButton.js Outdated
@@ -123,6 +130,7 @@ const RepairDeviceButton: React$ComponentType<Props> = React.forwardRef(function
desc={t("settings.repairDevice.desc")}
progress={progress}
error={error}
enableSomethingElseChoice={true}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
enableSomethingElseChoice={true}
enableSomethingElseChoice

https://reactjs.org/docs/jsx-in-depth.html#props-default-to-true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks! fixed in 3ed7e50

@alexandremgo alexandremgo merged commit 2136cc1 into develop Apr 29, 2022
@alexandremgo alexandremgo deleted the bugfix/LIVE-1317 branch April 29, 2022 08:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@juan-cortes juan-cortes juan-cortes approved these changes

@thomasrogerlux thomasrogerlux thomasrogerlux approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@alexandremgo @juan-cortes @thomasrogerlux
0