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

[LIVE-12107/LIVE-11325][LLD/LLM][Sync onboarding] Recover backup entry point #6645

Merged

Conversation

ofreyssinet-ledger
Copy link
Contributor

@ofreyssinet-ledger ofreyssinet-ledger commented Apr 10, 2024

βœ… Checklist

  • npx changeset was attached.
  • Covered by automatic tests.
  • Impact of the changes:
    • LLD Sync onboarding
    • React UI: Tag component

πŸ“ Description

  • LLD: Add a step "Backup with recover or keep manual backup of seed" in the sync onboarding. Cf. video below.
  • react-ui: Tag component: add "tiny" size, fix paddings that were too big.
backup.lld.mov

with the correct redirection in Recover app:

backup.recover.mov

❓ Context


🧐 Checklist for the PR Reviewers

  • The code aligns with the requirements described in the linked JIRA or GitHub issue.
  • The PR description clearly documents the changes made and explains any technical trade-offs or design decisions.
  • There are no undocumented trade-offs, technical debt, or maintainability issues.
  • The PR has been tested thoroughly, and any potential edge cases have been considered and handled.
  • Any new dependencies have been justified and documented.
  • Performance considerations have been taken into account. (changes have been profiled or benchmarked if necessary)

Copy link

vercel bot commented Apr 10, 2024

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

5 Ignored Deployments
Name Status Preview Comments Updated (UTC)
ledger-live-docs ⬜️ Ignored (Inspect) Visit Preview Apr 22, 2024 3:41pm
ledger-live-github-bot ⬜️ Ignored (Inspect) Visit Preview Apr 22, 2024 3:41pm
native-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Apr 22, 2024 3:41pm
react-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Apr 22, 2024 3:41pm
web-tools ⬜️ Ignored (Inspect) Visit Preview Apr 22, 2024 3:41pm

@live-github-bot live-github-bot bot added desktop Has changes in LLD mobile Has changes in LLM common Has changes in live-common ledgerjs Has changes in the ledgerjs open source libs translations Translation files have been touched ui Has changes in the design system library labels Apr 10, 2024
@@ -12,10 +12,7 @@ export const Base = styled(BaseButton)<{ big?: boolean }>`
line-height: 40px;
padding: 0 24px;

${p =>
p.variant === "shade"
? `background-color: transparent!important;border-color: currentColor;`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the border color override here was not matching the design

@ofreyssinet-ledger ofreyssinet-ledger force-pushed the feat/LIVE-12107-lld-synconboarding-recover-backup branch from feaf45b to 91b8256 Compare April 12, 2024 12:40
@jdabbech-ledger jdabbech-ledger force-pushed the feat/LIVE-12107-lld-synconboarding-recover-backup branch from 91b8256 to 39278f3 Compare April 18, 2024 14:59
@jdabbech-ledger jdabbech-ledger added the live-devices Device team label label Apr 18, 2024
@jdabbech-ledger jdabbech-ledger force-pushed the feat/LIVE-12107-lld-synconboarding-recover-backup branch 2 times, most recently from eefe393 to cca171e Compare April 18, 2024 15:11
@mbertin-ledger mbertin-ledger force-pushed the feat/LIVE-12107-lld-synconboarding-recover-backup branch from 5a89aed to 5fdf0ee Compare April 19, 2024 08:49
@mbertin-ledger mbertin-ledger changed the title [LIVE-12107][LLD][Sync onboarding] Recover backup entry point [LIVE-12107/LIVE-11325][LLD/LLM][Sync onboarding] Recover backup entry point Apr 19, 2024
@mbertin-ledger mbertin-ledger marked this pull request as ready for review April 19, 2024 08:51
@mbertin-ledger mbertin-ledger requested review from a team as code owners April 19, 2024 08:51
case "large":
default:
return "9px 10px 10px";
return "8px 9px 9px";
Copy link
Contributor

Choose a reason for hiding this comment

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

that could be a reason why e2e tests fail @mbertin-ledger

@jdabbech-ledger
Copy link
Contributor

jdabbech-ledger commented Apr 19, 2024

@jdabbech-ledger

Generating screenshots: workflow ended

Jobs status:
βœ… - generate-screenshots-linux

@ofreyssinet-ledger ofreyssinet-ledger requested a review from a team as a code owner April 19, 2024 09:57
@live-github-bot live-github-bot bot added the screenshots Screenshots have been updated label Apr 19, 2024
feat(llm/synconboarding): backup seed navigation

fix(llm/synconboarding): imports

fix(llm/synconboarding): params

feat(llm/SyncOnboarding): backup analytics

fix(llm/synconboarding/backup): Recover upsell redirection
@ofreyssinet-ledger
Copy link
Contributor Author

ofreyssinet-ledger commented Apr 22, 2024

@ofreyssinet-ledger

Generating screenshots: workflow ended

Jobs status:
βœ… - generate-screenshots-linux

Copy link
Contributor

@mcayuelas-ledger mcayuelas-ledger left a comment

Choose a reason for hiding this comment

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

Just console.log to remove

apps/ledger-live-desktop/src/storyly/useStoryly.tsx Outdated Show resolved Hide resolved
apps/ledger-live-desktop/src/storyly/useStoryly.tsx Outdated Show resolved Hide resolved
@ofreyssinet-ledger
Copy link
Contributor Author

ofreyssinet-ledger commented Apr 22, 2024

@ofreyssinet-ledger

Generating screenshots: workflow ended

Jobs status:
βœ… - generate-screenshots-linux

…on-reviews

πŸ’„  (lld/llm) [12240] Copy for recover during onboarding flow to be updated to latest version
color: "neutral.c70",
})``;

const VideoLink = () => {
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
const VideoLink = () => {
const VideoLink: React.FC<void> = () => {

Comment on lines 182 to 183
const BackupStep: React.FC<Props> = props => {
const { device, onPressKeepManualBackup } = props;
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
const BackupStep: React.FC<Props> = props => {
const { device, onPressKeepManualBackup } = props;
const BackupStep: React.FC<Props> =({ device, onPressKeepManualBackup }) => {

In addition, I think that it's better to rename the props by BackupStepProps to avoid a common Props as there is more than one component in the same file.

color: "neutral.c70",
})``;

const VideoLink = ({ onPress }: { onPress(): void }) => {
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
const VideoLink = ({ onPress }: { onPress(): void }) => {
type VideoLinkProps = { onPress(): void; };
const VideoLink: React.FC<VideoLinkProps> = ({ onPress }) => {

},
];

const BackupStep: React.FC<Props> = props => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same with this comment.

jiyuzhuang
jiyuzhuang previously approved these changes Apr 22, 2024
@ofreyssinet-ledger ofreyssinet-ledger merged commit d4135f1 into develop Apr 23, 2024
53 of 55 checks passed
@ofreyssinet-ledger ofreyssinet-ledger deleted the feat/LIVE-12107-lld-synconboarding-recover-backup branch April 23, 2024 09:06
@cksanders
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common Has changes in live-common desktop Has changes in LLD ledgerjs Has changes in the ledgerjs open source libs live-devices Device team label mobile Has changes in LLM screenshots Screenshots have been updated translations Translation files have been touched ui Has changes in the design system library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants