-
Notifications
You must be signed in to change notification settings - Fork 21
Conversation
f8cea7d to
199de3c
Compare
| ignorePatterns: [ | ||
| 'node_modules', | ||
| 'dist', | ||
| 'tailwind.config.ts', |
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.
Added to ignore pattern to resolve the following:
shared:lint: $ eslint '**/*.ts*'
shared:lint:
shared:lint: /PATH/blockchain-components/packages/shared/tailwind.config.ts
shared:lint: 0:0 error Parsing error: ESLint was configured to run on `<tsconfigRootDir>/tailwind.config.ts` using `parserOptions.project`: /PATH/blockchain-components/packages/shared/tsconfig.json
shared:lint: However, that TSConfig does not include this file. Either:
shared:lint: - Change ESLint's list of included files to not include this file
shared:lint: - Change that TSConfig to include this file
shared:lint: - Create a new TSConfig that includes this file and include it in your parserOptions.project
shared:lint: See the typescript-eslint docs for more info: https://typescript-eslint.io/linting/troubleshooting#i-get-errors-telling-me-eslint-was-configured-to-run--however-that-tsconfig-does-not--none-of-those-tsconfigs-include-this-file
shared:lint:
shared:lint: ✖ 1 problem (1 error, 0 warnings)
shared:lint:
shared:lint: info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
shared:lint: error Command failed with exit code 1.
shared:lint: ERROR: command finished with error: command (/PATH/blockchain-components/packages/shared) yarn run lint exited (1)
| <Text | ||
| as="h2" | ||
| className="sbc-max-w-full sbc-flex-1 sbc-text-ellipsis sbc-whitespace-nowrap sbc-break-all sbc-text-center sbc-line-clamp-2" | ||
| className="sbc-max-w-full sbc-line-clamp-2 sbc-flex-1 sbc-text-ellipsis sbc-whitespace-nowrap sbc-break-all sbc-text-center" |
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.
With the use of the updated package, the order for line-clamp also changed, moving the line-clamp in the className list. This is checked using the prettier package and this change was only made to resolve a prettier conflict.
@shopify/connect-wallet:lint: /PATH/blockchain-components/packages/connect-wallet/src/components/Modal/Modal.tsx
@shopify/connect-wallet:lint: 193:51 error Replace `flex-1·sbc-text-ellipsis·sbc-whitespace-nowrap·sbc-break-all·sbc-text-center·sbc-line-clamp-2` with `line-clamp-2·sbc-flex-1·sbc-text-ellipsis·sbc-whitespace-nowrap·sbc-break-all·sbc-text-center` prettier/prettier
@shopify/connect-wallet:lint:
@shopify/connect-wallet:lint: ✖ 1 problem (1 error, 0 warnings)
@shopify/connect-wallet:lint: 1 error and 0 warnings potentially fixable with the `--fix` option.
@shopify/connect-wallet:lint:
@shopify/connect-wallet:lint: error Command failed with exit code 1.
| <LazyMotion features={domAnimation}> | ||
| <AnimatePresence> | ||
| {qrCodeURI ? <QRCode uri={qrCodeURI} /> : <QRCodeSkeleton />} | ||
| </AnimatePresence> | ||
| </LazyMotion> |
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.
While adjusting the QR Code to utilize the built-in aspect-ratio functionality, I noticed that were were doing an animation here between QRCode and QRCodeSkeleton which actually had a little bit of a flicker of an unstyled box. This update resolves this "flicker" and instead just adds a <Spinner /> component in place of the QRCodeSkeleton.
| if (!uri) { | ||
| return null; | ||
| } | ||
|
|
||
| const matrix = generateMatrix(uri); | ||
| const {length} = matrix; |
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.
I moved the QR Code matrix generation into the memoized function here since I updated the type for uri to be optional.
| {dots} | ||
| </svg> | ||
|
|
||
| <div className="sbc-absolute sbc-inset-0 sbc-flex sbc-items-center sbc-justify-center"> |
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.
Changed sbc-bottom-0 sbc-left-0 sbc-right-0 sbc-top-0 to instead use sbc-inset-0 which is the same thing with fewer classes. Tailwind would've caught this and converted the className at compile time to use inset-0 instead, but this also cleans up our codebase.
| <m.div | ||
| animate={{opacity: 0}} | ||
| className="sbc-absolute sbc-inset-0 sbc-flex sbc-flex-col sbc-justify-center" | ||
| exit={{opacity: 0}} | ||
| initial={{opacity: 1}} | ||
| > | ||
| <Spinner /> | ||
| </m.div> |
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.
This is the Spinner container which will conditionally animate in and out utilizing the Framer presence functionality (animates the mount and dismount of the components to prevent from suddenly dismounting a component).
Soleone
left a comment
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.
Thanks for the thorough description and inline comments, much appreciated! Tophatted for Remix in Chrome using Metamask and works as expected, new Spinner into QR Code ✅
This also removes the use of @tailwindcss/aspect-ratio and @tailwindcss/line-clamp as they are now included in the Tailwind package. Additionally, this includes updates to the QRCode component to make use of the removal of @tailwindcss/aspect-ratio.
199de3c to
749a19a
Compare
ℹ️ What is the context for these changes?
Upgrades TailwindCSS to v3.3.2 to utilize built-in support for line-clamp and aspect-ratio. Additionally, this allows us to utilize TypeScript for our Tailwind config.
This also removes the use of
@tailwindcss/aspect-ratioand@tailwindcss/line-clampas they are now included in the Tailwind package.For
@shopify/connect-wallet, this includes a minor update to theQrCodecomponent displayed within theScanScreen. This change was made to address the removal of@tailwindcss/aspect-ratio. Additionally, theQRCodewas updated to make use of theSpinnercomponent which creates a better loading state while the WalletConnect URI is loading.🕹️ Demonstration
Screen.Recording.2023-07-10.at.11.59.31.AM.mov
🎩 How can this be tophatted?
yarn installyarn example✅ Checklist
Tested for accessibilityN/AIncludes unit testsN/AUpdated relevant documentation for the changes (if necessary)N/A