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

preliminary typescript rewrite and a bunch of fixes #348

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

Conversation

pocesar
Copy link

@pocesar pocesar commented Apr 5, 2019

The performance was subpar for my use-case, since the code relied on a lot of dynamic recreation of objects inside the render, and styles, so I decided to fix this (related #340). since I use Typescript as my primary language, went ahead and fixed a bunch of edge cases that would not be spotted in plain JS code (and also "fixes" #345, #270, related #276).
I'm also using forwardRef, so <SplitPane ref={ref} /> works. Had to export Pane and Resizer for the types to work.
The only issue is that I'm using React Hooks instead of classes, so this is definitely a breaking change for React projects stuck on old React versions. The code thrives on stable callbacks, that only useCallback hooks can provide, so people doing lambdas won't need to worry, since the inner callbacks are reference-stable.
It also fixes using the component on new windows and when using createPortal (fixes #336)

If this PR is unwanted, I can keep using my fork, no issues.

TODO: convert tests to TS and fix them

@thomasjm
Copy link

thomasjm commented Apr 9, 2019

As the author of #340 I love this. This seems like the right way forward and would make this library much nicer to maintain.

@pocesar
Copy link
Author

pocesar commented Apr 9, 2019

@thomasjm I just noticed the v2 branch, and although the code isn't much optimized (using spread props), the API is much nicer, should have seen that before rewriting the master 😅

@thomasjm
Copy link

Oh hmm, I wasn't aware of that either. I think pretty much everyone is still using 0.1.x though, and it's unclear when 2.x will be ready, so maybe it's still worth it to merge this.

@wuweiweiwu
Copy link
Collaborator

@pocesar this is awesome!

I'm currently working on getting v2 back into master! And having that in typescript would be amazing!

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.

Pane resizing doesn't work when the component is mounted in external window
3 participants