search

some-react-components / react-scrollchor

A React component for scroll to `#hash` links with smooth animations
ISC License
149 stars 24 forks source link

Is this package abandoned? I've got a fork that I could contribute back my changes from #39

Closed SeinopSys closed 3 years ago

SeinopSys commented 3 years ago

Considering there have been no updates to master in 3 years I figured this might be the case, but I am seeing relatively recent issue comments so I'm not sure what to think.

I was getting a bunch of warnings from React about the fact that "components using new hooks will not have their legacy hooks called" or something along those lines, and seeing the last publish date made me go the fork route, but I wanted to mention this here in case there are any other active maintainers who would like to discuss the possibility of contributing all or some of my changes back "upstream" so to speak.

My fork currently lives at https://github.com/SeinopSys/react-scrollchor and is published to NPM under a namespace of mine that I use for package forks. The key points I wanted to address, and then did so:

  1. Update all the dependencies/tooling to more recent versions (switched to Rollup instead of Babel)
  2. Keep API as same as possible (partially failed due to the removal of the default export for the component, because Rollup was throwing a warning about it, and also hooks support meant a higher minimum React version)
  3. TypeScript typings for everything (basically rewritten the package with types)
  4. Using the new function components with hooks

  5. Full easing function compatibility with jQuery easings (this may have been the case already but I checked the jQuery souce and adjusted the animation implementation to match how it works more closely, hopefully improving compatibility in the process)

    The complete set of changes are here: https://github.com/some-react-components/react-scrollchor/compare/master...SeinopSys:main

    I'd like to ask any remaining active maintainers to chime in and let me know if any of these changes interest you. I would be happy to prepare a separate PR just for those changes which you would like to have so that this package remains useful to all moving forward. It's often not trivial for developers to find where a package continues to live (if it even does) once it appears to be abandoned.

bySabi commented 3 years ago

Hey @SeinopSys

It is not abandoned, it is simply that no errors have been reported.

I have thought at some point of creating a new branch with Typescript and Hooks but I have not had time to create it.

I'm glad you took the time to do it for us. Your changelog is just great to me. It's what we planned to do. Except for the improvements in the easing function that had escaped me.

I would also like it to work with preact as well. What do you think?

We will leave the current 6.0.0 branch to support the olderReact versions and make the new master in the 7.0.0 release. We also need to give README a good refresher as well.

I'll add you as a mantainer if you like.

Thanks for collaborating.

SeinopSys commented 3 years ago

Personally I haven't worked much with preact, but when looking at the network graph I see that there is another fork which is specifically for preact: https://github.com/ltetzlaff/preact-scrollchor even thought it also appears to not be too regularly maintained.

I would personally prefer to keep the package React-focused just because that's where I'm actively using it and I cannot say that I would be checking it regularly for compatibility with other librbaries, and also managing the type situation could potentially be a nightmare. The only way I see forward for this package with regards to supporting other react-like libraries or just new features in general is adding a boat load of unit tests, of which there are currently none. That way the project could be checked against react, preact, or any other framework to make sure it still functions. I've done unit testing setups for a purely library package before, but this could be a bit more challenging due to the DOM element manipulation (scrolling) that needs testing here.

I don't know what a maintainer status here entails exactly, my intention was to just take my changes and make a separate branch + accompanying PR for this project and let you review and/or merge it as you see fit. Do I understand right that you would like to see all the changes in said PR but with a more verbose readme? Admittedly I "cheaped out" on the docs in that file because of the TS typing having such a wealth of info on them (in the form of doc comments) already, but I don't mind copying that information into the readme as well, it would just be extra work to keep in sync moving forward.

I'm not sure what you mean by the easings. The library code needed updating to match jQuery's implementation, specifically how the start and change parameters are hard coded to be 0 and 1 (source) and the unused first percent parameter is actually crucial for allowing a linear easing (sources: 1, 2). The existing user side custom easing functions that relied on the current call behavior would almost certainly be broken, but on the upside using any of the functions from either native jQuery or the jquery-easing plugin should only be a matter of copy-pasting them, that's exactly what I did for the linear and swing easing functions in my fork.

If you can confirm that you would like to see everything merged back I will prepare a PR of my main branch without all the package scope changes, otherwise please let me know if there is something from the set of changes that you would prefer I left out.

bySabi commented 3 years ago

I agree. At the moment we will only support React.

The README is a bit old. The Documentation is based on Prop types. We need to change that. I also agree that the new Typescript implementation would contain a part of the documentation as Types and Comments. But we can't just depend on that because many users surely use this component with only JavaScript. We will see it as we go.

React-Scrollchor has never aimed to follow any jQuery "standard". JQuery and easing are mentioned for guidance so that a non-expert user has some reference. I myself do not know much about these mathematical functions, I only limit myself to giving the facility that in API users could provide the easing function that would suit them best. If you want to give a more formal support to this, I think it's great.

In I agree to all changes. The only thing I would not agree with is to change it to remove the name React-Scrollchor, it is his best achievement :-)

I'm glad that someone like you took the time to improve this component. When I wrote it, there weren't any that had the API I had in mind. Right now it has about 44K monthly downloads. With your PR it will surely go to more.

I add you as a mantainer, if that's okay with you.

Onr more thing. If all goes well here, I'm sure I ask for your help to give a little "love" to two packages that I think many people would find useful with a little more attention, hookleton y garfio. If you dare.

SeinopSys commented 3 years ago

Pull request has been created, let's discuss any additional specifics with regards to my existing contributions there.

I'm sure I ask for your help to give a little "love" to two packages

You can ask, but I'm not really one to volunteer to help with something I'm not using myself. I'm contributing to this package specifically because I found it useful for my own needs and I wanted to improve it.