Skip to content

Conversation

@julio-salas03
Copy link

Before diving into the PR changes, I would like to thank you for this awesome library. I've been using it a lot lately and I love it ❤️.

Main request

Currently the SplitText only recalculate all the wrappers when the window is resized [1]. Although this could be enough for most use cases, if the actual component is resized the text will overflow the container.

Fixing this is really simple. You just need to replace the window.addEventListener('resize', onResize) with a ResizeObserver as done in this PR.

I also prepared this codepen so you can see the package's current behavior vs the PR code.

Other enhancements

While working on the ResizeObserver I noticed some aspects of the code that could be improved:

The SplitText component

  • It seems like you're the key state to "force" a re-render on the SplitTextInner, but this is unnecessary
  • There's no need to separate the resize and the wrapping logic into 2 components (SplitText and SplitTextInner respectively)

The SplitTextInner component

  • After the first call to refreshLines, the maxCharPerLine was never updated, which could cause issues when the component gets bigger on a resize, as the old value will prevent the lines from expanding even though the container would expand.
  • Overall, the code was a little confusing. For instance, the refreshLines function received 2 parameters which were state values, so you could just read them from the component.

Although these are not critical, I truly believe they could help improve the quality of the library.

Notes

I noticed you had some git hooks that ran the project's tests on push. I ran into an issue where ResizeObserver was not defined on the test environment. Since I'm not very familiar with the test runner you're using, I ran my git push with the --no-verify-flag. I wanted to get this PR out and hopefully get some pointers about how to get around/fix this issue with the tests

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.

1 participant