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

feat: Support TypeScript #106

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

condorheroblog
Copy link

First of all, I'd like to thank you for your contribution.

Now, I need to use this codebase in a TypeScript environment. I've already migrated to TypeScript, and I'd appreciate any suggestions you might have.

Closed #98

@condorheroblog
Copy link
Author

If you want to release a new version, just run the npm run release command.

@tuckergordon
Copy link
Collaborator

@condorheroblog sorry for the delay here. I'm technically a maintainer now and would be interested in getting this merged in, however it looks like your fork messed with the prettier formatting which resulted in a lot of files being changed.

If you're still interested in this, please adjust it back so that it matches up with the existing formatting and reduces the number of changes in this PR

@condorheroblog
Copy link
Author

Thank you for your reply, yes I am still interested, I used this project on company's official website.

I used Biome to replace ESLint and Prettier for code formatting.

If we want to use ESLint and Prettier, we need to upgrade to the latest version. ESLint from 8 to 9 is a major version upgrade. Can you create a commit to upgrade the formatting tool?

@tuckergordon
Copy link
Collaborator

👍 yup i'm working on upgrading all the project deps right now and will try to hit eslint as part of that. I'm also using this in some work for my company and was running into the same issue with eslint

@condorheroblog
Copy link
Author

After you upgrade ESLint, you can modify my code directly or notify me and I will modify the code. Thank you.

If you don't want to configure ESLint one by one, you might consider using https://github.com/antfu/eslint-config.

@tuckergordon
Copy link
Collaborator

Okay I opened up #107 - give that a try and lmk if you run into any issues. If that works, then i'd suggest using that branch as a starting point and then adding in the typescript support. Hopefully that'll be fairly straightforward with all the changes I made

Copy link
Author

@condorheroblog condorheroblog left a comment

Choose a reason for hiding this comment

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

Hi, I solved the code conflict, now you can continue to check my code. Thanks😁

Copy link
Collaborator

@tuckergordon tuckergordon left a comment

Choose a reason for hiding this comment

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

Hey just did a high level review and in general things look good and there are a lot of great changes in here! I can do a more in depth code review but before I do that, hoping you can address a few things:

  • Lots of prettier changes in here, which is my fault for forgetting to add a .prettierrc. Can you please add this .prettierrc to the file and re-run. Otherwise it's too difficult to review this PR because almost every file has changes in it
{
  "bracketSameLine": true,
  "printWidth": 100,
  "proseWrap": "always",
  "singleQuote": true,
  "tabWidth": 2,
  "useTabs": false
}
  • I appreiciate the desire to add some infrastructure around releases and think it's a good idea, though I'm not sure we should implement it until we get some response from @jsonkao. He has the credentials for npm so we're not going to be able to publish anything until then
  • Thanks for switching the example to vite, babel was a pain
  • Also thanks for creating tests! Though I'm getting a React Intersection Observer was not configured to handle mocking. message when trying to run them

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like you're using an npm mirror, which is fine but we should be committing those changes to the package-lock. I can run npm install and push a non-mirror version if that's easier for you?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I have removed the mirror address (because the Chinese government restricts free access to the Internet, so I have to use the mirror source 😂)

src/step.tsx Outdated
}, [ref.current]);

const childElement = Children.only(children);
return cloneElement(childElement, { ref: setRefs });
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a type error here that prevents build

Copy link
Author

Choose a reason for hiding this comment

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

👌 fixed

@condorheroblog
Copy link
Author

Also thanks for creating tests! Though I'm getting a React Intersection Observer was not configured to handle mocking. message when trying to run them

react-intersection-observer changed some code, I need some time to understand why 😄

Check this link: thebuilder/react-intersection-observer#689 (comment)

@condorheroblog
Copy link
Author

react-intersection-observer author released a new version (v9.16.0), upgraded to the latest version, solved the warning information in the test.

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.

Providing Types for TypeScript?
2 participants