Add data context example with filtering/sorting/pagination about fixed-data-table-2 HOT 36 CLOSED

Hey @gforge, use of context within FDT is already supported and we use it heavily at Schrodinger for applying realtime updates to our FDT. We do it by wrapping our Table in HOC which implements the getChildContext method. Then in our cell's render methods we can access this.context as expected.

In order to make sure updates to context aren't swallowed, we also have our HOC track a revision value which increments whenever the context changes. That revision value is passed as a prop to our cells.

I'm not sure if passing the revision is compatible with you case of cells being stored outside of the table, but that might be a simple place to explore extending FDT. Our pagination example uses the dataVersion field in a similar manner to this.

While this approach can get you to a dynamic table based on context, one downside of this approach is that every update to the context will rerender all cells. In our own application we went a step further and created a CellFactory which is a HOC of a cell, which pulls out all the necessary data for rendering the cell from the context and passes that data into the actual cell renderer as props. This means each cell can do it's own diffing for performance and not be affected by the context across all cells.

@KamranAsif designed the system, so I bet he has more details to add if you're interested. If possible, could you try the approach described and see how far you can get? Then we can work on handling any blockers that you face, such as needing to pass a contextRevisionId from the Table component into all cells.

from fixed-data-table-2.

@wcjordan that sounds great. I've changed the title to indicate that an example(s) that uses context should be added. Since context has a lot of barb-wire around it, I would as a junior JS developer feel comfortable that people with more skills are recommending this particular solution. Also, if it breaks in future versions I will know where to start looking for an updated solution.

The context tracker, is that for solving the shouldComponentUpdate issue? The main problem with context is that it shouldn't be updated after mounting, as I understand it from that article.

from fixed-data-table-2.

Ya, the big issue with context is if context changes it may not cause rerenders in the components which rely on it, which would result in an out of date DOM. Take for example a case of A which renders B which renders C (A -> B -> C). If A provides a context which is used by C but not B, an update to the context may be swallowed by B's shouldComponentUpdate method, and not propagate to C.

There's no risk of this occurring where A is your HOC, B is FixedDataTable, and C is your Cell, because FDT internally only uses shouldComponentUpdate to save renders while scrolling. It doesn't try to avoid renders when data hasn't changed and leaves that to the individual cells, but we might change that in the future.

This could happen though if your cells internally did a should component update check. The difficulty there is that deep comparing a large context object is expensive, especially when run for every cell in the grid. We use the revision approach I mentioned above so that we only do this deep compare once per context update.

Often folks recommend not changing context after mounting since that would avoid all these issues, but we want dynamic data from the context, so we need to go a bit further.

from fixed-data-table-2.

What do you think of an example to guide us n00bs? I can give it a try if you don't have something that you can share out of the box.

from fixed-data-table-2.

I would take the pagination example and remove PagedData from the cells and the state and instead provide a method in PaginationExample which looks like:
/** @inheritDoc */ getChildContext() { this.pagedData; },

Removing PagedData from the state should look something like making the constructor look like
`
constructor(props) {
super(props);

this._updateData = this._updateData.bind(this);
this.pagedData = new PagedData(this._updateData);
this.state = {
  end: 50
};

}
`
and removing from cells should just be deleting the data prop.

I think that will get you pretty far, so play around with that and let me know where you see issues.

from fixed-data-table-2.

So I did an attempt for a context-based paginator: https://github.com/gforge/fixed-data-table-2/blob/context/examples/PaginationExample.js It seems to do the job. I've changed the code to a more clean ES6-version and I moved the two cells into the example. I'm not a big fan of the cells being located in a separate location, they are rather simple in structure and as a new user it is nice to have all the code available. If you disagree I can revert to the prior style before any PR.

I think I understand the basic concept but I think that it would be great with some more comments on:

• an explanaiton to the _updateData - as I understand it it updates the state with the new end-data that triggers a re-rendering of the pending status
• what is the benefit of having a PagedCell component and a PendingCell - why do we need the dataVersion to be passed into Pending?

from fixed-data-table-2.

I've further played around with the pagination example and there are a few strange things that I noted in the original code that I've managed to remove while keeping the example working (see my fork example linked above). The changes are thus:

• The components have been converted to functions a'la ES6
• The context is added via a HOC called DataCtxt
• The _updateData seems inappropriately named. It seems that the setState is simply there for triggering a re-rendering. In my example I've therefore changed the name to refresh for better conveying its purpose.
• I kept searching the for the functionality of this.state.end only to realize that it isn't used. I've therefore added a version counter that triggers the re-rendering, an alternative would be to call forceUpdate but that seems "not so reactish".
• The PendingCell didn't seem to have any functionality to it that I could understand and has been removed.
• The _dataVersion and its associated function didn't seem to have any crucial functionality and was removed without any errors.
• The callback is now set by the HOC - this makes it easier to understand for n00bs and more in-line with how I imagine that the component will be used in true applications.
• The HOC has moved into a helper file as I'm starting to work on a filter-example.
• The PaginationExample now uses a composed class, DataTable that takes data as a prop.

Have I missed something vital? Do you want to update the current example to this new alternative or do you want to have two parallel examples?

from fixed-data-table-2.

Awesome! Thanks for putting this together! Here's some feedback on moving this forward:

I think you should open a PR with your changes; that will be a good place for revision discussions since comments can be coupled to the code.

Very excited to have ES6 style functions and functional components where possible. Bonus points if you can make the other examples consistent.

HOC logic looks good from an initial glance, and naming changes seems fine. In particular the transitiion from a variable named end to using the revision is good to have.

A performance regression will occur from removing PendingCell and having PagedCell not inherit React.PureComponent. Now we'll re-render cells on every scroll of the grid which may look fine in the example but will cause jank on larger grids. I would change it back. _dataVersion was used for passing the PureComponent checks on updates, but in your example we could use the version counter prop instead. I think we may need to add support to pass it through though.

I also see some inconsistency in breaking the example into an HOC and moving it to a new class when compared to moving the cell renderer into the file. It seems it will cause the same confusion and lack of availability of example code as you mention above. I would recommend we standardize on keeping logic in the example, except when it's shared by other examples. This should result in moving the HOC back into the example pending my suggestion below.
Downside of this would be cells won't be all in the same place since some are specific and others are shared...
@KamranAsif any feelings on this point?

I'd like to keep an independent example for context from the existing PaginationExample. Since context is discouraged by React and more complicated to set up, I think we should make it an advanced example and discourage it's usage unless a user has a sufficient need for it.
I also think we should have only a single context example. So if you'd like to show using context for both paging and filters, we'd have that in a single example rather than multiple. I think that's the best way to ease new users into the library. I'm open to discussion if you feel differently.

Also let's be sure the description for the context example points to this issue and states that it's currently being dogfooded / only has moderate support.

from fixed-data-table-2.

Sorry I was absent from this discussion; I was on vacation for last week and a half.

Everything here looks really good so far. I agree we want it as a separate example.
I was also not a huge fan of putting the cells in their own files. I rather copy + paste them and manually update them all instead of forcing people to look them up.

As Chris mentioned, you need to pass the version into the component. I don't see why it needs to be a react function; a react component should work fine.

from fixed-data-table-2.

Hmm, I'm against copy and pasting the cells since that makes it hard to keep the examples up to date...
I do think that helpers/Cells work could use work to be more approachable.

Maybe we need a simple primer / documentation page on what a Cell should be and then we don't expect a user to infer that from the examples.

from fixed-data-table-2.

Thanks for the feedback. Here's my plan and thoughts:

• Subfiles: As I started working on the FitlerExample I decided to move the HOC and context cells into separate files. As a new user I would love to get as much of the example presented at once. My plan is to split into multiple files at the moment and then recombine them once the examples are working. That way I know that the same HOC/cell structure is implemented all-over. Ideally we would have a smart "code-example" presenter that builds the dependencies but that would require some work.
• PendingCell: I'll see how to best recreate the PendingCell - I don't think I entirely grasp why PureComponent doesn't re-render while the current solution does. As the user scrolls does that trigger a rendering event that is propagated all the way down to the cell? The intuitive behavior would be that the scroll updates the y/x-positions and leaves the internal parts of the cell intact. Regardless, I think adding more comments to the functionality of this element is necessary.
• Separate context examples: Sounds like a good idea. I'll move the current code into separate files and create new links. I'll prepend all the context examples with Ctxt or should we have a separate folder?
• Context elements: In the future I think it would be great if HOCs and other elements here from the examples were part of the exported code base. One could for instance export an object named Context that has it's own version of Table, Column and Cell.
• ES6: I'll start updating, not sure I'll have the time for all examples but I'll do it for the examplest that I need for my work. Should we start a separate issue for this? It would be nice to also link this update with adding a linter-style.
• PR: I'll start one once I'm done with the FilterExample - the code is currently a little of a mess :-(

from fixed-data-table-2.

For understanding PureComponent, I recommend reading React's optimizing performance post. The propagation of renders all the way down the component tree is a React principle, where any change to a component re-renders all subcomponents. The PureComponent and other stuff in optimizing performance are used how to avoid that in cases where you know it is safe. This is something Cells need to do as FDT does not try to infer it for you.

Also I'm against exporting part of examples as the code base since we don't have the momentum to support them and want the support burden to be on the user. An alternative might be for you to support and independent library for this which we reference in the tutorials and show usage of in the examples. But we could still avoid handling maintenance of it as part of FDT.

One other point which I think got lost is, I recommend just moving towards a single context example with both filter and paging. We can create a new section of 'Advanced' examples to hold it and future cases which go beyond the practices we can easily support.

from fixed-data-table-2.

Thanks for the link. Interesting read.

The exporting examples is more of a future vision - I agree that it may be too much at the moment.

Moving everything into a single advanced example file will make it hard to navigate - it would mean that a user has to scroll through a set of examples to get to the one of interest. The code would similarly be rather lengthy and difficult to traverse, or do you propose a refactoring of the example page?

from fixed-data-table-2.

Sorry I didn't mean to have all examples in one page. I meant we should aim to have only one context example and have that example show both filtering and paging in a single example.

from fixed-data-table-2.

Ok, I understand. That's where I'm heading with my experimentation.

I re-implemented the PendingCell and did a simple test for the rendering:

const PendingCell = function() {
  let i =  0;

  return class extends React.PureComponent {
    render() {
      i = i + 1;
      const {data, rowIndex, columnKey, dataVersion, ...props} = this.props;
      console.log(i, dataVersion)
      const rowObject = data.getObjectAt(rowIndex);
      return (
        <Cell {...props}>
          {rowObject ? rowObject[columnKey] : 'pending'}
        </Cell>
      );
    }
  }
}()

Unfortunately the counter indicated similar activity (as I moved up and down ) regardless of the PureComponent - have I missed something in the re-implementation?

from fixed-data-table-2.

I can't tell without the full code running in a debugger. What you'd need is to ensure no props or state are changed on the PureComponent. If that's the case it should not re-render.

from fixed-data-table-2.

Created an advanced example where the Filter + Sort worked nicely together. Adding the Pagination messed things up, as expected. Not sure how I should tackle this. After loading the new data, the sort + filters should be invoked again. I've played around with some solutions that haven't worked:

• I tried adding a _callback to filter/sort that was merged using nested functions (crashed the browser :-P)
• I tried adding a refresh to each layer with each layer having its own internal version numbering (the current version of the code) but I get a similar error

Any suggestions would be great.

from fixed-data-table-2.

Managed to solve the issue although I think that sorting a paginated dataset on the client-side is not recommended. I added a note about that in the example.

A few general questions that remain:

• There is now a single advanced example - do we stick to this or do we keep an example of context without the pagination?
• There are now three HOC that are located in the helpers/HOC.js - do we want to move them into the code? Keeping them separate has the advantage of allowing a simpler import into a project by copy-pasting the entire file.
• All the HOCs have identical refresh() and _getDataWrapper methods. The natural way would be to inherit a state but I know that's frowned upon. I read something about inverted inheritance but that seems like a weird design choice. Any suggestions?
• There is some naming that I'm uncertain of
  • refresh or triggerUdate?
  • DataCtxt or AddDataCtxt - I've used AddFilter and AddSort - it seems like a preferable alternative.
  • setCallback or onDataUpdate - the latter seems more intuitive

Thanks for all the help and patience with this! U guys are awesome!

from fixed-data-table-2.

Nice. I'll try and look sometime this weekend and get a better idea for cleaning things up.
I agree that including sort + paging is a bad idea. Why don't we just drop 1 of them from the example?

Since we have independent examples for each without context hopefully a reader can infer mixing things if that situation arises.

from fixed-data-table-2.

I guess we would have to drop sorting since paging is the challenging part. Should we leave the code for those that are interested? I'm thinking of having a HOC.js merging a set of HOCs that are located within a HOC-folder. This folder would also contain the DataListWrapper class. This has though the downside of making the code less browseable...

I'm also considering if it wouldn't be good to have an additional parameter to getObjectAt - the parameter would indicate if a fetch should occur or not. Currently the entire dataset is filtered and not just the section that is displayed - probably an important feature once you start scaling to really large datasets.

I've updated the HOC-file and adapted to the Airbnb style guide. To match the style guide requirements I added the except package in order to remove properties that should not be passed to sub-elements. What's your status on adding your own style guide?

from fixed-data-table-2.

I think you've done a good job here. We should scope things down to ensure this doesn't become a distraction from project goals. Could you push the rest of what you have to a branch and I'll make a pass to cut things down make it a more newbie friendly example? Other further enhancements may make sense as a separate project which builds on top of FDT. It's important to keep examples as a starting point for new users and not an all encompassing solution.

Wrt to style guides, the code is mostly still in the format the original Facebook authors used, and hasn't been cleaned up. Airbnb style guide is a good one to follow though so I'm sure we'll be happy with that.

from fixed-data-table-2.

If you create a contexExample-branch I can change the PR to that. I've started to collect all the random stuff into a fixed-data-table-addons package. We could reference this for those that want to dive further into the world of random HOCs.

What are your thoughts on the naming issues that I mentioned?

from fixed-data-table-2.

I don't think there's a need for a separate branch. Is everything in the state you want it on gforge:master?

I don't know about the other questions yet. I'll need to go through the code more to have an idea.

from fixed-data-table-2.

Ok. I cleaned out the other examples and removed the sort.

from fixed-data-table-2.

I'm trying to implement a test-suite for the addons-package. I'm not entirely sure how to best approach testing this rather complex structure and I don't fully grasp the current tests in fdt-2 (I've only found the ReactTouchHandler-test which isn't exactly n00b friendly).

I've therefore tried to implement a more plain Chai/Mocha approach. I've figured out the basics but I'm not sure how to check if the table contain the cells that I expect it to. Any suggestions of what I'm doing wrong? The example code is here: https://github.com/gforge/fixed-data-table-addons/tree/master/test

from fixed-data-table-2.

I won't have too much time this week, but will look if possible

from fixed-data-table-2.

No worries, I wrote up a question on StackOverflow. Enzyme seems like a good option.

from fixed-data-table-2.

Awesome. Enzyme is really good. We use it for testing React components internally at Schrodinger and it's recommended by the React core team, so it's likely a good way to go.

from fixed-data-table-2.

Ok, I have a pretty decent test-suite set-up and the package is on NPM (my first npm-package!). I haven't figured out istanbul/coveralls so if you have a hint that would be awesome.

There is now a branch that uses the addons-package in case you prefer not having all that HOC code within fdt2.

from fixed-data-table-2.

Awesome, I like the enhancement library!

I've created a PR with some recommended clean ups before we accept this here:
gforge#1

I also think decorating the React component to do the filtering isn't the best way to do this since the filtering doesn't need to be tangled with the grid. Instead, I think we may want to decorate the store to accept filters and filter the values directly. What are your thoughts on that approach and do you have any time to attempt a solution like that?

from fixed-data-table-2.

The PR has been merged.

I did a solution with putting the filtering into redux but I had weird refresh issues (ended up with a looping issue) - probably just my JS n00b status. While I understand the concept of data being processed by the reducers, it does decouple the logic. I found that defining filters in the reducers was intuitively less convenient than doing that in the component together with the actual filter forms. I prefer this solution as I view the filter to play nicely with FDT and becomes a contained unit in my mind.

For now I'll try to focus on improving the addons-package. It took me a while to realize that I needed to "compile" before publishing and I would like to get the coverage part working. I'll play around a little with the current implementation and if I you data-decorator pattern more intuitive I'll change and do a new PR.

from fixed-data-table-2.

Word, let's not block this on the filter stuff then. If this is good to go, I'll merge this in and we can revisit pushing all the filter logic into the store down the road.

from fixed-data-table-2.

Merged the PR. I'll redeploy the docs and close this issue tonight. Thanks again for the contribution and all the hard work!

from fixed-data-table-2.

Awesome! Thanks again for the patience with all my questions.

from fixed-data-table-2.

Also when you're ready to share or gain more interest in your add-ons package, send us a PR to mention / link to it from the context example. I think that would be a nice addition to allow folks to explore further and show off your work.

from fixed-data-table-2.

How can i detect upScroll?
And how can i pass rowIndex in onVerticalScroll callback function as a parameter?

from fixed-data-table-2.