prometheusresearch-archive / react-grid

MIT License
80 stars 20 forks source link

Adding keyboard nav, tests, gulp, react 0.11.1 #12

Closed bakesteve closed 10 years ago

bakesteve commented 10 years ago

Cleaned this up a bit, but the main changes are:

Structure / tooling

ExcelGrid wraps the Grid component and adds keyboard navigation support Toyed with a number of ways of doing this, but the one we've gone for here is:

Partly as a way to get the grid working in react 0.11.0, which broke in the context and child components (being descriptors) see To make this all work, reduce complexity, and tweak some extra performance, got rid of the custom scrollbars, and revert to the Divs scroll May have missed some crucial details, but in our tests this wouks well (bar a small issue on horizontal scroll keeping the headers in track #9) Primarily this revolved around removing the scrollbar components, and the DOMMetrics mixin Let me know if there are areas I misunderstood / it has introduced regressions

React v0.11.1

As above, upgrading broke the DomMetrics mixin, so I chopped it out

Tests

Added tests (runnable via gulp) and a jasmine testRunner.html file had to hack the gulp task a little so that I could get browserify to pick up requires properly. We plan to migrate to webpack anyway, so will revisit that then The tests are split into core and browser, with anything under core getting run in node (via gulp) We're planning on plugging Jest in to get a fake DOM and then shoudl be able to make most tests run in browser OR in node, so we can have a watch that automatically reactifys, es6ifys, jshints, and runs tests

The tests so far are mostly on the keyboard nav side, but clearly lots of areas to add specs on for the main grid

bakesteve commented 10 years ago

Closes #8

bakesteve commented 10 years ago

@andreypopp here you go appologies its all in 1 big go, but I cleaned the commits a little

As I said - we thought this repo was dormant so may have been more radical / opinionated on tooling etc than was warranted, but you can now do gulp and launch a local site or gulp tests-run to launch the test runner (need to cleanup a few files for jshint to make the console less angry..)

Let me know your thoughts on this, both from a tooling / tests point of view, and on the react side of things for keyboard nav, and for the scrolling. We are relative newcomers to React, so may have taken some odd paths / done things in the wrong way

we've also listed what we'd like to see happen to extend this here - and be happy to collaborate on getting a roadmap pulled together

jackofseattle commented 10 years ago

I'm curious as to why we would need to merge excel functionality into the base grid? Doing so breaks the single use premise of the component world and starts to create some hybrid jquery plugin landscape.

Can we not just have a second component that's not part of this that adds in the excel functionality? Ultimately we'd end up with something much more light weight and modular.

jackofseattle commented 10 years ago

Sorry to hijack the PR. We are debating using this on a new project at the company and I'm always concerned about library size. Thanks for fixing the 0.11 issues though, that's super helpful.

bakesteve commented 10 years ago

@jackofseattle – yes, I did wonder if this wasn’t better placed as an extension My initial attempt was closer to this, but ended up having to either fallback to DOM events/listeners too much or replacing large parts of the grid (ie rowRenderer)

The only real changes required to the Grid were to add SelectedCells and Row / Cell click events. So maybe a better solution is to either:

If you can see a way to do this in more of a behaviour based way (using mixins / controllers I’d assume), that’d be great

For editors, with a few tweaks to allow passing full objects to the cellRenderer, that felt do-able without touching the Grids internals, cell selection though didnt

bakesteve commented 10 years ago

@jackofseattle no worries. and size is always important! Not sure what your build pipeline is - but if you are using browserify/webpack/etc then if you just require('./Grid') you wont get any more than a few bytes extra with the extra keyboard navigation stuff

bakesteve commented 10 years ago

@andreypopp Any initial thoughts on this? If it helps can split out the gulp vs tests vs navigation vs v11 stuff?

On the main part of adding selection onto thegrid by click or keyboard, I've also done a attempt at a version as it feels more of a behaviour than anything else but had 2 key issues:

At the moment, a cell renderer gets the column and the value of your data object/array For complex renderers we may need more info. Should we just let people stuff an object here? So data[row][colKey] ={ Value:123, CurrencyCode:"usd", Styles: ... Cx: ... } That feels the most flexible to me and keeps the basic use case of passing in a flat object simple. Only extension I can think of is to allow value to be a function,to allow better composition ad functional lenses etc? In the case of passing in if a cell is selected, it would mean we could have: function() { return { value:"abc", IsSelected:function(column, rowData) { Return Selcted && Selected[rowData.key] && Selected[rowData.key][column.key] } } }

The (potential) issue I can think of here is working out should component update at the cell level, as without evaluating both the lens and isSelected() you can't tell.

Let me know your thoughts on directions for this, as well as if it would help to break out this pr One thing for us that will help is using grunt/gulp not makefiles

Cheers steve

bakesteve commented 10 years ago

Given the grid has now changed since we have created our master, going to close this down and submit properly (ie feature based pull requests, not 1 big behemoth)