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

[DSDK 175] Add LedgerHQ UI library #8

Merged
merged 8 commits into from
Jan 30, 2024

Conversation

aussedatlo
Copy link
Contributor

@aussedatlo aussedatlo commented Jan 18, 2024

πŸ“ Description

Adding the Ledger LDD Design System to the sample app, remove tailwindcss and configure styled-component.
Inspired by next.js styled component example

❓ Context

βœ… Checklist

Pull Requests must pass the CI and be code reviewed. Set as Draft if the PR is not ready.

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


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

Copy link

vercel bot commented Jan 18, 2024

@aussedatlo is attempting to deploy a commit to the LedgerHQ Team on Vercel.

To accomplish this, @aussedatlo needs to request access to the Team.

Afterwards, an owner of the Team is required to accept their membership request.

If you're already a member of the respective Vercel Team, make sure that your Personal Vercel Account is connected to your GitHub account.

@aussedatlo aussedatlo force-pushed the feature/DSDK-175-ui-library branch from 51a28f6 to 794fb71 Compare January 19, 2024 09:55
import { GlobalStyle } from "@/components/globalstyles";
import type { AppProps } from "next/app";

export default function App({ Component, pageProps }: AppProps) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we only use naming exports ?
An eslint rule could be useful for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this file, next require a default import. But I agree we should use naming exports when possible.

Copy link
Member

Choose a reason for hiding this comment

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

Why use only named exports @jdabbech-ledger ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Named exports are more restrictive as they require you to use a specific identifier when importing. Default exports are more flexible, you can use any name during import but may lead to inconsistencies in the codebase.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @aussedatlo on this point, moreover the IDEs follow more easily the import with naming exports in general

apps/sample/tsconfig.json Outdated Show resolved Hide resolved
@aussedatlo aussedatlo force-pushed the feature/DSDK-175-ui-library branch from 794fb71 to 92eea72 Compare January 19, 2024 13:09
* used to customize the rendering of pages. For more information, refer to the Next.js
* documentation on customizing the App component:
* https://nextjs.org/docs/advanced-features/custom-app
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Great, thank you for the added doc ;)

@aussedatlo aussedatlo force-pushed the feature/DSDK-175-ui-library branch 3 times, most recently from e656c39 to 3b24209 Compare January 30, 2024 16:25
@aussedatlo aussedatlo force-pushed the feature/DSDK-175-ui-library branch from 3b24209 to aa845e1 Compare January 30, 2024 16:33
@aussedatlo aussedatlo merged commit 2326785 into develop Jan 30, 2024
2 of 3 checks passed
@valpinkman valpinkman deleted the feature/DSDK-175-ui-library branch April 17, 2024 16:26
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.

4 participants