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.

Sign up for GitHub

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

Jump to bottom

refactor(lld): nfts gallery medias in newArch #6784

Merged
merged 3 commits into from May 10, 2024
Merged

refactor(lld): nfts gallery medias in newArch #6784

merged 3 commits into from May 10, 2024

Conversation

LucasWerey
Copy link
Contributor

@LucasWerey LucasWerey commented Apr 30, 2024 •
edited

✅ Checklist

  • npx changeset was attached.
  • Covered by automatic tests.
  • Impact of the changes:
    • None

📝 Description

Refactor of nfts gallery medias inside newArch to prepare the integration of ordinals and improve readability
Change from class components to RFC.
Creation of the folder Collectibles where all collectibles will be centralised
Creation of a FF to hide the implementation of newArch
For the moment none of the components added in NewArch are in use

❓ 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)

@LucasWerey LucasWerey requested review from a team as code owners April 30, 2024 12:27
@vercel Vercel
Copy link

vercel bot commented Apr 30, 2024 •
edited

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 30, 2024 4:48pm
ledger-live-github-bot ⬜️ Ignored (Inspect) Visit Preview Apr 30, 2024 4:48pm
native-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Apr 30, 2024 4:48pm
react-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Apr 30, 2024 4:48pm
web-tools ⬜️ Ignored (Inspect) Visit Preview Apr 30, 2024 4:48pm

@live-github-bot live-github-bot bot added desktop Has changes in LLD mobile Has changes in LLM labels Apr 30, 2024
@live-github-bot live-github-bot bot removed the mobile Has changes in LLM label Apr 30, 2024
@KVNLS
Copy link
Member

KVNLS commented Apr 30, 2024

Could you use a Feature Flag to make this migration, please? As we did for the Market page on LLM we need to be able to roll back to the previous architecture without the need to do a new release. You can use the Router to display the correct page whether the FF is activated or not. Plus we need to avoid references to the newArch in the old one.
You can duplicate components if needed and we will remove them once we know the migration is working well in a second time.
Thanks in advance!

lvndry
lvndry reviewed Apr 30, 2024
View reviewed changes
apps/ledger-live-desktop/src/newArch/Collectibles/components/CollectionName.tsx
import { useNftCollectionMetadata } from "@ledgerhq/live-nft-react";
import { Account, ProtoNFT } from "@ledgerhq/types-live";
import NFTCollectionContextMenu from "~/renderer/components/ContextMenu/NFTCollectionContextMenu";
import Skeleton from "~/renderer/components/Nft/Skeleton";
import { Skeleton } from "LLD/Collectibles/components";
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the difference between LLD and ~ ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LLD is the alias for ~/newArch/

apps/ledger-live-desktop/src/newArch/Collectibles/components/index.tsx Outdated
@@ -0,0 +1,6 @@
export { default as Skeleton } from "./Skeleton";
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just use named exports in the files themsevles ?
And do export { Skeleton } from "./Skeleton"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done ✅

@LucasWerey LucasWerey added the WIP Work in progress label Apr 30, 2024
@LucasWerey LucasWerey marked this pull request as draft April 30, 2024 15:56
@live-github-bot live-github-bot bot added common Has changes in live-common ledgerjs Has changes in the ledgerjs open source libs labels Apr 30, 2024
@LucasWerey LucasWerey removed common Has changes in live-common ledgerjs Has changes in the ledgerjs open source libs WIP Work in progress labels Apr 30, 2024
@LucasWerey LucasWerey marked this pull request as ready for review April 30, 2024 16:31
@live-github-bot live-github-bot bot added common Has changes in live-common ledgerjs Has changes in the ledgerjs open source libs labels Apr 30, 2024
@LucasWerey LucasWerey changed the title refactor(lld): nfts gallery in newArch refactor(lld): nfts gallery medias in newArch May 3, 2024
@LucasWerey LucasWerey removed the request for review from a team May 7, 2024 08:18
@LucasWerey LucasWerey merged commit 69dfce5 into develop May 10, 2024
61 of 74 checks passed
@LucasWerey LucasWerey deleted the refactor/medias_nft_gallery branch May 10, 2024 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lvndry lvndry lvndry left review comments

@gre gre gre approved these changes

Assignees

@LucasWerey LucasWerey

Labels
common Has changes in live-common desktop Has changes in LLD ledgerjs Has changes in the ledgerjs open source libs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@LucasWerey @KVNLS @gre @lvndry @beths-ledger