-
Notifications
You must be signed in to change notification settings - Fork 315
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
Feat/migrate v3 onboarding #593
Conversation
The latest updates on your projects. Learn more about Vercel for Git βοΈ
|
π¦ Changeset detectedLatest commit: 6e33e62 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
c0d7f3d
to
a9a7db9
Compare
82f7224
to
604194c
Compare
604194c
to
51aede9
Compare
Codecov Report
@@ Coverage Diff @@
## develop #593 +/- ##
===========================================
+ Coverage 43.14% 47.84% +4.70%
===========================================
Files 572 650 +78
Lines 24416 29121 +4705
Branches 6633 7532 +899
===========================================
+ Hits 10534 13933 +3399
- Misses 13837 15130 +1293
- Partials 45 58 +13
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Screenshots: β
There are no changes in the screenshots for this PR. If this is expected, you are good to go. |
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.
Don't be scared by the quantity of comments.
It's mainly some questions and details. I didn't see any major issues except for one thing:
There are way too many styled components that could have been just simple Design System components with the correct props.
Ideally we shouldn't need any custom CSS on LLD anymore, not for V3. This probably won't be always possible but we should always do it whenever it is.
I understand this is the accumulation of several people's work, so most of those aren't even yours. And I don't want to delay the merge of this PR any more than we have to. But I do think we should tackle this ASAP as a team. Otherwise they will just multiply due to the chameleon rule.
apps/ledger-live-desktop/src/renderer/components/DeviceIllustration.tsx
Outdated
Show resolved
Hide resolved
`; | ||
|
||
const NanoSP = styled.div` | ||
// TODO: rendering issue in the SVG in the "hole" |
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.
not sure what this means? was it fixed? can you put maybe a more explicit comment about what the issue is?
apps/ledger-live-desktop/src/renderer/components/Illustration.tsx
Outdated
Show resolved
Hide resolved
apps/ledger-live-desktop/src/renderer/components/Illustration.tsx
Outdated
Show resolved
Hide resolved
...dger-live-desktop/src/renderer/components/Onboarding/Screens/SelectUseCase/UseCaseOption.tsx
Outdated
Show resolved
Hide resolved
...dger-live-desktop/src/renderer/components/Onboarding/Screens/SelectUseCase/UseCaseOption.tsx
Outdated
Show resolved
Hide resolved
apps/ledger-live-desktop/src/renderer/components/Onboarding/Screens/Welcome/Carousel.tsx
Outdated
Show resolved
Hide resolved
apps/ledger-live-desktop/src/renderer/components/Onboarding/Screens/Welcome/index.tsx
Outdated
Show resolved
Hide resolved
apps/ledger-live-desktop/src/renderer/components/Onboarding/Screens/Welcome/index.tsx
Show resolved
Hide resolved
51aede9
to
f09df2c
Compare
f09df2c
to
9595b06
Compare
apps/ledger-live-desktop/src/renderer/components/Onboarding/Help/HideRecoverySeed.tsx
Show resolved
Hide resolved
apps/ledger-live-desktop/src/renderer/components/Onboarding/Help/PinHelp.tsx
Show resolved
Hide resolved
apps/ledger-live-desktop/src/renderer/components/Onboarding/Help/RecoverySeed.tsx
Show resolved
Hide resolved
9595b06
to
aa519d9
Compare
23bbe75
to
07c6411
Compare
/generate-screenshots |
π Description
Migrate v3 onboarding to the main repository
Figma Link
β Context
ledger-live-desktop
β Checklist
πΈ Demo
simplescreenrecorder-2022-07-15_11.28.09.mp4
π Expectations to reach
Please make sure you follow these Important Steps.
Pull Requests must pass the CI and be internally validated in order to be merged.