carbon icon indicating copy to clipboard operation
carbon copied to clipboard

Convert packages/react storybook to typescript. Convert checkbox story to tsx

Open mbarrer opened this issue 3 years ago • 11 comments

Closes #11587

Changes have been made to allow the use of typescript within packages/react so that storybook stories can start to be written in typescript. As part of this change, the main.js for storybook has now been converted to main.ts and has some initial typings. Typing files have been added for Checkbox that we're sourced from the DefinitelyTyped repo and adjusted for carbon v11

Changelog

New

  • "typescript": "^4.7.4"
  • "@typescript-eslint/eslint-plugin": "^5.30.5" and "@typescript-eslint/parser": "^5.34.0" for linting
  • "ts-node": "^10.9.1" for supporting imports in storybook main.ts
  • typings/shared.d.ts common types file pulled from DefinitielyTyped
  • components/Checkbox/Checkbox.d.ts new typing file for Checkbox (adjusted DT typings)
  • custom.d.ts ambient module to allow imports of .mdx files in .tsx
  • tsconfig.json

Changed

  • update mini-css-extract-plugin to support native ts typings
  • converted .storybook/main.js to typescript and allow stories to be written in typescript
  • converted components/Checkbox/Checkbox.stories.js to tsx using the new typings
  • Added linting for .ts, .tsx to eslint-config-carbon/plugins/react.js

Testing / Reviewing

In order to test:

  • fresh yarn install
  • new build
  • launch storybook and verify that the Checkbox story is working and has the same functionality as it had before

mbarrer avatar Aug 23 '22 16:08 mbarrer

DCO Assistant Lite bot All contributors have signed the DCO.

github-actions[bot] avatar Aug 23 '22 16:08 github-actions[bot]

Deploy Preview for carbon-components-react ready!

Built without sensitive environment variables

Name Link
Latest commit cfef450b7b07f2c15647be18fdfab0fa3e091ca8
Latest deploy log https://app.netlify.com/sites/carbon-components-react/deploys/6328a237aeaec6000847ed06
Deploy Preview https://deploy-preview-12000--carbon-components-react.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify[bot] avatar Aug 23 '22 17:08 netlify[bot]

Deploy Preview for carbon-elements ready!

Name Link
Latest commit cfef450b7b07f2c15647be18fdfab0fa3e091ca8
Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/6328a237f4719800083d252e
Deploy Preview https://deploy-preview-12000--carbon-elements.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify[bot] avatar Aug 23 '22 17:08 netlify[bot]

I have read the DCO document and I hereby sign the DCO.

mbarrer avatar Aug 23 '22 17:08 mbarrer

@mbarrer Are you using Yarn v1 by chance? The project is configured to need [email protected]. I believe this is why the deploy previews are failing. I think if you update/install to v3.2.1 and re-run yarn install from the root things should start working.

Since you added new dependencies there should also be some additions to .yarn/cache as well for you to commit and push up in addition to the yarn.lock

tay1orjones avatar Aug 23 '22 20:08 tay1orjones

Ah yes I have yarn 1.22.18 installed, I can update this on my end and run a fresh yarn install. I didnt see any .yarn/cache changes but that may just be because my version is so old

mbarrer avatar Aug 23 '22 21:08 mbarrer

@mbarrer Hey so out of curiosity I pulled down this PR, reset yarn.lock to what's currently in main, and then re-ran yarn install. This pulled the changed files down from ~1800+ to just 26. I think the snafu with yarn v1 inadvertently reset all the hashes on the yarn cache, which is why it was such a huge change set.

Anyway, should be fixed now 👍

tay1orjones avatar Aug 24 '22 19:08 tay1orjones

Looks like the build task also needs to be adjusted to output a main.js file for use in netlify, will be taking a look at this

mbarrer avatar Aug 26 '22 17:08 mbarrer

Just letting you guys know that I haven't forgot about this PR, have just been busy this week with story work. I'll aim to have the build issue resolved by the end of the week

mbarrer avatar Aug 30 '22 22:08 mbarrer

Seems like the issue I was hitting was something that @erifsx brought up in a thread on the workgroup slack. We found that we didnt need ts-node to get things running initially but for some reason this is now required in order to run the main.ts. Adding this to the dev dependancies seems to resolve the issues with netlify

mbarrer avatar Sep 02 '22 18:09 mbarrer

@jdharvey-ibm updated PR based off your recent comments

mbarrer avatar Sep 19 '22 16:09 mbarrer

Closing in favor of https://github.com/carbon-design-system/carbon/pull/12222

tay1orjones avatar Nov 01 '22 12:11 tay1orjones