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

PF4 Bring Reactabular in-house #1628

Closed
priley86 opened this issue Mar 22, 2019 · 6 comments
Closed

PF4 Bring Reactabular in-house #1628

priley86 opened this issue Mar 22, 2019 · 6 comments
Milestone

Comments

@priley86
Copy link
Member

priley86 commented Mar 22, 2019

Reactabular is no longer as active as the project here and I believe thru discussions and recent PRs it would be better to bring this minimal library in-house. Reactabular's current source is a good starting point for writing minimal table extensions, but I believe we can customize and improve it much faster here. The upstream maintainer is no longer able to actively maintain it as noted in #313 #338.

Noted issues causing a poor developer experience:

  • onChange \ onBlur callbacks for rows are not handled correctly upstream and our current workarounds are not great.
  • passing refs thru callback methods is no longer a good way to pass React refs, and React.createRef() should be used instead.
  • getChildContext is the old way of using React context. We can update this to use the new stable Context API.

Proposed solution:

  • We only currently consume reactabular-table (which is luckily only 12 js files 😎). Bring them in-house and expose as a new package @patternfly/reactabular-table and consume them from @patternfly/react-table. I think this would be a great first step. Another alternative (which may be better and help us avoid another internal package) would be to bring the source directly into react-table and just rename the components to avoid conflict, i.e. BaseProvider, BaseBody, BaseBodyRow, BaseHeader, BaseHeader, BaseHeaderRow or attempt to integrate them inside our current wrappers there.
  • Continue writing our own extensions here in patternfly-react

Additional nice-to-haves:

  • convert to TSX
  • incrementally fix the issues above
  • revisit the API of Provider, Body, BodyRow, Header, HeaderRow and see if we can improve them.

thoughts?

@atiratree
Copy link

Noted issues causing a poor developer experience:

Agree. I would also note that this is about the user experience as well. It is impossible to implement real-time validation (key by key) with current reactabular-table .

We only currently consume reactabular-table (which is luckily only 12 js files). Bring them in-house and expose as a new package @patternfly/reactabular-table and consume them from @patternfly/react-table. I think this would be a great first step.

This approach makes sense and is a good way forward.

@redallen
Copy link
Contributor

If we're going to go through that much effort, it would be nice to compare other 3rd party table dependencies too.

@atiratree
Copy link

If we're going to go through that much effort, it would be nice to compare other 3rd party table dependencies too.

+1 it is good to look at other options. Anyway I think other solutions might be hard to customize for our API and take much longer to adapt compared to just modifying reactabular. Also the consumers are quite used to how reactabular handles things

@priley86
Copy link
Member Author

priley86 commented Mar 22, 2019

@redallen - this was discussed originally in 402. I think we used Reactabular initially as a "how-to" for building minimal React tables, but now we have outgrown it in a sense. Many other 3rd party libraries impose assumptions that I don't think PF quite warrants (i.e. take Bootstrap as an example, we no longer subscribe to that ecosystem for various reasons). In many cases, it makes more sense to take these implementations in-house and customize them in PF (and treat JS similar to the way we now treat CSS in PF Next). This should be a minimal effort (what I am describing above), but always happy to hear about alternatives 😸

@priley86
Copy link
Member Author

priley86 commented May 6, 2019

resolved in #1250 - the hope is to update the base components and convert them to TSX in the future. This was necessary to resolve some React warnings being thrown in the base library. I have also gone ahead and converted lodash usages to lodash-es.

@priley86
Copy link
Member Author

closed by #2011

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 a pull request may close this issue.

4 participants