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

Functional Components, TypeScript Support, and Vite migration #101

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

Conversation

emil-sharkov
Copy link

This is a huge pr with a lot of changes so feel free to give constructive feedback and I will try to address.

  • While migrating to functional components I was able to solve a bug where innerRef was not being utilized by ScrollSyncPane properly if it was passed a function.
  • Typescript, functional components, and Vite migrations were to modernize the library

Copy link
Owner

@okonet okonet left a comment

Choose a reason for hiding this comment

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

Thanks a lot for taking on it. I was planning to do so but ended up re-implementing the library for a client. I think community could benefit from these changes. It would be great if you could address the feedback I left.

There are a few questions that needs to be resolved: notable the release process is not clear now. Previously I used semantic release AFAIK. How will the releases be managed with this change?

Also, are there any breaking changes?

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/components/ScrollSync.tsx Outdated Show resolved Hide resolved
src/components/ScrollSync.tsx Outdated Show resolved Hide resolved
src/context/ScrollSyncContext.tsx Outdated Show resolved Hide resolved
src/context/ScrollSyncContext.tsx Outdated Show resolved Hide resolved
src/context/ScrollSyncContext.tsx Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
@okonet
Copy link
Owner

okonet commented Dec 13, 2024

I upgraded Node to v20 on netlify so let's see if it will be able to build after that. Push some changes to trigger a deploy preview please

@emil-sharkov
Copy link
Author

There aren't any "breaking changes" in this library. A bug fix around innerRef and some modernization. Personally, I am fine with keeping semantic releases but I am open to learning changesets if you could help me with it

@emil-sharkov emil-sharkov requested a review from okonet December 14, 2024 05:30
.editorconfig Outdated Show resolved Hide resolved
package-lock.json Outdated Show resolved Hide resolved
@emil-sharkov
Copy link
Author

Would you consider adding the source code for the docs website here? I think it could use some modernizing: faq, more examples, etc

@okonet
Copy link
Owner

okonet commented Dec 17, 2024

The website code is here and is driven by styleguidist. I would keep it if possible

@okonet
Copy link
Owner

okonet commented Dec 17, 2024

Can you please check netlify build logs. Something is wrong with some dependencies.

@emil-sharkov
Copy link
Author

Could you add me to your netlify team? I think the issue is that it is caching old dependencies and we need to clean install

@emil-sharkov
Copy link
Author

The website code is here and is driven by styleguidist. I would keep it if possible

styleguidist is not compatible with vite as far as I know. I lean towards Storybook as an easy way to generate docs. I would be open to creating the necessary stories and infra in the repo

@okonet
Copy link
Owner

okonet commented Dec 17, 2024

I see... yeah feel free to switch to storybook or another tool of your choice. Maybe a separate PR for that and then rebase this one on top?

@okonet
Copy link
Owner

okonet commented Dec 17, 2024

I tried to add you as a contributor but I don't know how to do that. Netlify mentions git contributors but again no idea how to add you.

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.

2 participants