TanStack / table

🤖 Headless UI for building powerful tables & datagrids for TS/JS - React-Table, Vue-Table, Solid-Table, Svelte-Table
https://tanstack.com/table
MIT License
24.98k stars 3.07k forks source link

v7 Feedback & Ideas #1252

Closed Grsmto closed 5 years ago

Grsmto commented 5 years ago

Hi, First, I'm trying v7, it's awesome to work with Hooks! Trying it for the first time so my comments might be wrong.

Describe the bug This are more of an architecture comment than a bug: If you disable useExpanded hook, the table breaks. I know in userland you can't disable this hook but the fact that this happens, (edit: actually you can) probably demonstrates wrong app design: I figured out this is happening because you're building initial state within the useExpanded hook (row.cells = ...).

Expected behavior I think that adding or removing hooks should not break the entire component. Hooks should be encapsulated.

Codesandbox! https://codesandbox.io/s/nk9rmym6m4

Solution That initial state logic could be within useTable itself (tested and works fine). I can provide a PR if needed.

tannerlinsley commented 5 years ago

I totally agree. I've done my best so far to make them isolated, but there are still some artifacts like this one you discovered. I'll rework that hook to be more isolated.

Grsmto commented 5 years ago

Hey, Sorry to comment here again regarding v7 architecture but I don't know if you guys have an open discussion somewhere about it so I put my thoughts here:

Hook clashes

I think it needs a pattern for hooks clashes: currently the different react-table hooks APIs are mutating the different react-table props to expose the hooks API: ex:

This approach has different issues:

Proposed solution:

Let me know if this helps, I would be happy to provide some more contributions if you guys are actually open to discussion!

Thanks!

tannerlinsley commented 5 years ago

This is valuable feedback. I'll definitely take it into consideration as I'm flexing the API over the next few weeks.

tannerlinsley commented 5 years ago

Some notes while it's on my mind:

More to come.

Grsmto commented 5 years ago
  • I will not be possible to add core hooks to the table system. (They could clearly mutate api.hooks to add their own anyway

Why not? For example when I have been experimenting with the v7, I have been adding a useSelectRow hook to add support for select/unselect and batch actions on the table. It's been working just great! I feel like you miss an opportunity here to provide a "plugin-ready" table where users could add their own feature via hooks.

  • If we're designing a system that will encourage users to request more and more hooks to be added to the core, then we're doing something wrong. Up to this point, I don't think we are, because....

Yeah that's why a plugin system is great, if a user needs more feature, he can just add it himself without spamming the maintainers.

I think the main issue at the moment with the architecture is that the hooks are mutating the data. I think that hooks should act more like "reducers" (as in Redux reducers for example), take the data as param and return new (mutated) data. That way you could let the users add hooks without worrying for the table to break. That's pretty much what the hooks are doing at the moment and I'm not sure why that mutation is necessary.

tannerlinsley commented 5 years ago

I think I see what you're referring to now.

The first api is a reducer pattern, which is essentially what produces the final API. It's started in the useTable hook, then reduced through each plugin hook. Each plugin is responsible for returning the newly decorated or changed API as a replacement. Nothing is mutated there.

As all of the hooks are being reduced down to the final API, an internal hooks: {} object is currently used to keep track of functionality that must be deferred and applied later (after the final API has been reduced). Maybe hooks is a bad name internally, since it conflicts with the actual React hooks vocabulary. I'll try and think of a better one. Anyhow, the mutations you're referring to like api.hooks.getTableProps.push(...) could easily be avoided by creating a method like api.addHook('getTableProps', fn). Still, this technically wouldn't protect a user from mutating the hooks list though, since it is a property of the api that is being reduced. Again, we could pull that list out and only pass down the addHook function, but I feel like it's a bit overkill right now.

Under those circumstances, I think you're very correct that in addition to adding more functionality to existing hooks (like adding a prop to a header) you'll also want to be able to add new hooks (and their accompanying methods) to the system like what you mentioned: <input {...row.getSelectInputProps()} />. That way the user would have a new method to call in their markup AND other hooks could extend that api by using api.addHook('getSelectInputProps', fn).

Did I miss anything?

janewang commented 5 years ago

[Feature] Could we have a hook for update entire columns? Right now we are bounded to cells and rows level updates. We want to pivot the table which would make it awesome, is this possible?

Aj1402 commented 5 years ago

[Feature] We can add support for virtualized rendering when used without pagination, this can increase performance of rendering huge tables without pagination.

gary-menzel commented 5 years ago

@Aj1402 RT7 is headless - the developer using the library handles the rendering. The sample @tannerlinsley provided on codesandbox (https://codesandbox.io/s/m5lxzzpz69) shows how infinite scroll can be implemented by creating a hook. Check src/components/Table.js and look for useInfiniteScroll - as well as just clicking the buttons on the demo. I don't think it is guaranteed, but that hook may be supplied as part of a set of hooks in the production release.

jackpope commented 5 years ago

Are there docs in the works for v7? I realize it is still in alpha release but it will help get more usage and feedback. I am trying to bring v7 into my project (for the first time, never used prior versions) and am having to jump into the source to figure out how to use the APIs. For example, I'm trying to fire a callback when filters change and see onFilteredChange referenced in the docs/issues but don't see it in the code base.

tannerlinsley commented 5 years ago

Docs are still in the works unfortunately.

Also, the concept for handling changes to state have changed drastically from v6's onFilterChange:

// Hoist the table state out into your own code
const state = useTableState({ pageCount: 0 });

// Grab the piece of table state you want to watch
const [{ filters }] = state;

// Use an effect that triggers when it changes
useEffect(
  () => {
    // filters changed
  },
  [filters]
);

// Pass the hoisted state to the table
useTable({
  ...
  state,
  ...
})

Essentially, you hoist the table state out into your own code which allows you to watch it for changes with useEffect probably being the easiest way to do this.

dbertella commented 5 years ago

What is the best way to start and play with version 7, I'm looking forward to see all the possibility this rewrite will open and I would like to start testing it and possible contributing to it. I believe the only example out there is the code sanbox linked above, correct? Right now I'm trying to make a table with a nice treeview and I'm wondering if it can be a good feature for v7 and easier to implement instead of hacking something on top of v6 (I tried to use the HoC in v6 but is not doing a great job at the moment)

gary-menzel commented 5 years ago

@dbertella - it will definitely be easier (in some ways) to do a Tree View in V7 but, as with all tree data, it is going to depend on how your tree is organised. I am still learning V7 myself but hope to add a tree view example. You would need to write a hook to manage the state (i.e. nodes open/closed etc.) and would possibly also need to implement other hooks to manage what "sorting" or "filtering" means against your tree data. I recommend having a look at the useExpanded hook as it is currently used to do the pivoting node management.

gary-menzel commented 5 years ago

For others following along here, I have recently been able to get a pure <table>, <tr>, <td> renderer going but there are a few more changes to test and PR to the core before that could be released in a codesandbox.

dbertella commented 5 years ago

@gary-menzel thank you for the quick answer! I really want to dig into it now even more, I just find it difficult to figure out how to start. I guess I will start from the sanbox and get a simple version running, it might be nice to have a working example inside the repository perhaps.

gary-menzel commented 5 years ago

That's what I have done. Created a CRA, got the files from the codesanbox, installed the V7 ReactTable and then just poked in. I have not done much with hooks but the basic concept of them is simple to understand - it is just that the hooks for RT7 do a LOT. There are also some "bugs" still to iron out. It should be possible to remove a lot of the hooks and just get a basic table (and it will be) but there are a few dependencies still lurking between the hooks.

gary-menzel commented 5 years ago

@dbertella as far as an example in the repo - I think that will end up staying clean and there will be Wiki entries and codesandbox samples (easier than previously because you can have teams in codesandbox and bind things to github). I am also in the process of planning a e-book that will (hopefully) cover everything.

Jessy-BAER commented 5 years ago

Hi,

@gary-menzel I try to use v7 with server side data and infinite scrolling but it doesn't work even here https://codesandbox.io/s/m5lxzzpz69 :)

// When sorting, filters, pageSize, or pageIndex change, fetch new data
  useEffect(() => {
    fetchData();
  }, [sortBy, filters, pageIndex, pageSize]);

pageIndex never changes

gary-menzel commented 5 years ago

I'm still learning V7 myself and have not yet gotten into how best to approach server-side (or infinite scrolling). @tannerlinsley would probably be the best one to help you out with this (although we are still also deciding what out-of-the-box hooks and renderers will be provided with V7 - maybe we can get something in that just works - or we might rely on some community input once the rest of the API has settled down).

berdyshev commented 5 years ago

I like the new hooks approach!

I'm curious if useExpanded should be used only for aggregation/pivoting but not expand/collapse of the rows to display details of the row?

btw, it seems that the row expanding is not working in the demo https://codesandbox.io/s/m5lxzzpz69 because row.toggleExpanded is not defined.

tannerlinsley commented 5 years ago

useExpanded is actually extremely lightweight, so it's okay to use it for simple row expansion. Its useGrouping that is responsible for aggregation/pivoting.

Yeah, I need to update the sandbox with what's in master.

berdyshev commented 5 years ago

@tannerlinsley thank you for your response. Can you please tell me how to use useExpanded for simple row expansion?

tannerlinsley commented 5 years ago

Remember, all of this is in alpha and pretty hazy right now, but essentially, using the useExpanded hook will give you various .toggle() methods and expanded state that you can use to control a row's expanded state. You can also read from a row's expanded state through properties on the row model like isExpanded.

That should be adequate for implementing any custom UI for expanded rows you want, inside or outside of your actual table.

berdyshev commented 5 years ago

@tannerlinsley thank you again. When do you plan to update the npm package and the demo with the latest code?

revskill10 commented 5 years ago

Thanks very much for a great library. v6 is awesome for what i experimented. For V7, it uses React Hooks. And i think this is a real deal for a React Datatable! The real deal is in the simplicity, composability of Hooks, so let's prove it, right ?

Codesandbox is great and helpful, but it's not the best way to prove the change and improvement. Let's prove it in the README. Just give all API usage in a README , nothing more.

My experience with API design in general is that: If you can't put a working example in a README file, there's something more complicated than it should be.

I tried out current codesandbox examples for V7, and my feeling is like: oh my god, is this too complicated ? Or is it over-engineering, or am i too simple ? I don't know. The example doesn't hook me. I'm not sure about it, i want it to be more simple to use.

berdyshev commented 5 years ago

@tannerlinsley

using the useExpanded hook will give you various .toggle() methods and expanded state that you can use to control a row's expanded state. You can also read from a row's expanded state through properties on the row model like isExpanded.

As I understand there is no API to manage the rendering of the expanded rows, right? (maybe I can help with it in case you want it to be a part of the library?)

I ended up with something like this:

const MyTable = ({ expandable = false, renderExpandedRow, ...props }) => {
   const instance = useTable(props, ....);
   .....

   const renderRow = (row, index, style = {}) => {
    if (!row) {
      return (
        <Row {...{ style, even: Boolean(index % 2) }}>
          <Cell>Loading more...</Cell>
        </Row>
      );
    }
    prepareRow(row);
    const rowProps = row.getRowProps({ style, even: Boolean(index % 2) });
    const renderedRow = (
      <Row
        {...rowProps}
        expanded={row.isExpanded}
        onClick={() => {
          if (expandable) {
            row.toggleExpanded();
          }
        }}
      >
        {row.cells.map(cell => (
          <Cell {...cell.getCellProps()} className={cell.column.className}>
            {cell.render("Cell")}
          </Cell>
        ))}
      </Row>
    );

    if (row.isExpanded) {
      return (
        <React.Fragment key={rowProps.key}>
          {renderedRow}
          <Row
            {...rowProps}
            className={classNames(rowProps.className, "row--expanded")}
            key={`${rowProps.key}_expanded`}
          >
            {renderExpandedRow(row.original)}
          </Row>
        </React.Fragment>
      );
    }
    return renderedRow;
  };

  return <TableView ... />;
}
tannerlinsley commented 5 years ago

So maybe the example isn't very clear on this, but expanded rows are not nested in the model, they are flattened into list of total rows. When you look at each row, you will see row properties that denote the depth of the row, parents, etc. This makes it 10x easier to deal with nested data and not have to write recursive renderers.

React Table strictly does not provide any markup to the user for this reason. What you have here could work great for your specific use case, but might not be what someone else needs. If we were to add it to the API, we would be 10x-ing the API by adding the needed inversion of control points that any given user would need to fully customize it.

Ultimately the v6 API is massive and extremely bloated, and yet it still doesn't support a fully customized rendering option. v7 is half the size and supports an unlimited number of rendering options. This is a perfect example of why headless UI components are so great!

berdyshev commented 5 years ago

So maybe the example isn't very clear on this, but expanded rows are not nested in the model, they are flattened into list of total rows. When you look at each row, you will see row properties that denote the depth of the row, parents, etc.

I'm not sure but it seems you are talking about aggregation/grouping of the rows. But I'm trying to implement something like in this example https://codesandbox.io/s/github/tannerlinsley/react-table/tree/master/archives/v6-examples/react-table-sub-components.

tannerlinsley commented 5 years ago

Most of what I said remains the same. The expanded state that is used for grouping/aggregation can also be used for sub components. It all depends on how you choose to render the expanded state.

berdyshev commented 5 years ago

@tannerlinsley would you be able to publish the new version with the latest code, please?

guru-florida commented 5 years ago

My feedback: We've been using v6. I just reviewed the examples of v7 and I have some limited experience with hooks. I think v7 is looking awesome but it is also a huge sea-change from v6. So much so that IMO it should be a new github project not an upgrade from 6 to 7. I am concerned how many users of ReactTable are going to be surprised when they upgrade 6 to 7 on a whim and the API is completely changed. Not 30% changed, but 100%! lol. Could you instead create another github project and then refer ReactTable users to the new project going forward in the README? Maybe "React-Tabular" or "React-Cell" or "React-xcell" haha.

I think you could then adapt current ReactTable API to use the new v7 project as a dependency and provide the user visuals on top of the hooked ReactTable but would actually have a v6 compatible API. Basically swapping out the old v6 behavior code for the hook code. Existing ReactTable users would experience only a bump in the road going to v7 but would also have a stepping stone to switching to their own visual styles using hooks. Would this be the best of both worlds?

I have my doubts people on my team are going to understand hooks anytime soon, they can barely wrap their heads around React vDOM :(. It's unfortunate, but it is the truth of most developers and companies that like to hire cheap "talent" (but they do understand v6 React-Table).

This is food for thought. I really appreciate your work into ReactTable! Both v6 and v7 are awesome!!

tannerlinsley commented 5 years ago

@guru-florida that's good feedback. To help with your concern, my plans for v7 are not to only release what you currently see today as the v7 headless core, but also build and release a migration-centric kitchen-sink component along side it. This component's purpose will be to almost replicate the API and experience of the v6 component, but using the new v7 core under the hood. My hope is that this will help people migrate to v7, but easily be able to break out of the "training wheels" component when they are ready.

As for your concern with hooks, I feel obligated to put in my 2 cents: Hooks are React. I wouldn't consider them an addon or alternative syntax. They are core, just like the component API. While this may be painful, it shouldn't be any different than when people were forced to learn the HOC or render-prop patterns, except for they'll be learning core React patterns and utilities that will make their react code safer, faster, easier to write/compose/extract/share, and aligned with the rest of the React ecosystem. I know this can be a hard pill to swallow for companies or larger orgs with many employees, but trust me, your future selves will applaud you for learning and writing new code in React hooks ASAP.

Thanks for your feedback! Keep it coming!

guru-florida commented 5 years ago

@tannerlinsley Thanks. I wasn't aware of your migration plan and it's essentially the same thing so glad to hear.

I agree 100% with your second paragraph too but it's easier said than done in my case. I don't have control over who is hired so I have to deal with the hand I've been given. TL;DR version they just aren't that interested in learning it. Old habits. What can ya do right? ....just move on....so in 3 more days I move on to another company. for reals. haha :)

I already use HOC and hooks, etc so I loved the new v7 design immediately and would migrate to it now but no time with me gone in 3 days. I am sure I'll be using it on the next project though.

AskAlexSharov commented 5 years ago

Feedback:

Ciantic commented 5 years ago

Have you considered TypeScript?

Having third party type definitions for a very important component like tables (data grids) can be a bit unnerving.

With official type definitions when user of the react-table updates the library, the user can be somewhat certain the update does not break the behavior since it type checks.

tannerlinsley commented 5 years ago

@Ciantic Yes, I have. A few things hold me back on that though:

I have no qualms with moving to TS when the time is right, but in the meantime I will encourage integration tests for stability in its stead.

tannerlinsley commented 5 years ago

@AskAlexSharov, would you like to submit a PR to improve those experiences?

dmitriigaidarji commented 5 years ago

@tannerlinsley I am trying to sort desc on first user click. In v7 passing defaultSortDesc=true as a prop to the Table or Column only allows to flip between 'sort desc' and 'sort unset' states, which makes it impossible to sort ascending.

Is this a bug or am I misusing the property?

tannerlinsley commented 5 years ago

Sounds like a bug.

Tanner Linsley On May 14, 2019, 6:11 AM -0600, Dmitrii Gaidarji notifications@github.com, wrote:

@tannerlinsley I am trying to sort desc on first user click. In v7 passing defaultSortDesc=true as a prop to the Table or Column only allows to flip between 'sort desc' and 'sort unset' states, which makes it impossible to sort ascending. Is this a bug or am I misusing the prop property? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

dmitriigaidarji commented 5 years ago

@tannerlinsley thanks for the quick response! I've fixed it for myself and shared the code for you #1341

strass commented 5 years ago

When do you think we will see docs for v7? I'd like to use it but it's a little overwhelming without any help or typescript bindings.

tannerlinsley commented 5 years ago

I'm writing the docs right now as I'm upgrading Nozzle from v6 to v7 in our next big release. Once we launch our next version of Nozzle, I'll consider its use-cases complete enough and the API design sufficient to finish the docs. Plan on a month.

rssluca commented 5 years ago

@tannerlinsley would you be able to publish the new version with the latest code, please?

Been digging into v7 code for a bit, keen to properly understand how to use the depth/path properties manually to aggregate rows into expandable sections. Would also like to display the parent row when group is collapsed. I saw groupByFn and aggregations properties in useGroupBy but not sure yet if they are related..

eladbs commented 5 years ago

@tannerlinsley First, thanks again! When copy&pasting table data to another place (word, excel, etc..) it is missing tabulation (table structure). Will it be possible to add support for it in v7? Is it even possible technically with css flex?! Thanks!

gary-menzel commented 5 years ago

@eladbs The CSS Flex implementation will not work (with any page using it - not just only react-table) as the Office tools are looking for <table>, <td>, etc. That being said, RT7 allows you to create your own rendering - it's headless, rendering is done by the developer and there will be extra examples along with both the flex/div layout and one using table but anything is possible.

So, if you want people to be able to mark the page and copy into Word/Excel, then you can provide a renderer based on the traditional <table> tags.

eladbs commented 5 years ago

@gary-menzel Thanks for your reply! You mean that I can render each component in the table with the appropriate table/td/tr/etc. to support that capability?

Somehow, I hoped that there will be built in support. I know it's hard or even impossible.

gary-menzel commented 5 years ago

@eladbs Rendering is completely isolated from the table engine itself (which is all based on hooks).

So, while RT7 will have some basic renderers in the box, the developer is ultimately able to decide how to render the table. RT7 essentially manages the composition of the data so that it fits the desired "shape" (i.e. the columns - and the page of rows).

It's not hard or impossible - it's just not really a design goal of RT7 to be opinionated about how to render tabular data. The big challenges with previous versions of ReactTable were that everyone wanted different outcomes from the rendering. So, RT7 provides that.

eladbs commented 5 years ago

@gary-menzel Cool. Thanks! I'll take a look at RT7 and will try to accomplish that.

ryanking1809 commented 5 years ago

I'm loving this headless architecture! Just a note, the codesandbox provided in the docs is a little buggy as it's not using the latest release, but I've managed to fix all the issues I was having by switching to the npm package. Where can I find the latest code for future debugging? I seem to only be able to find alpha 2.

Grsmto commented 5 years ago

@ryanking1809 there is alpha 6 released on Npm https://www.npmjs.com/package/react-table. Would be nice if you could provide that code sandbox link here maybe so other people can use it to debug etc.!