Skip to content

Adds the ability to revert an edit by hitting the escape key.#22

Merged
nadbm merged 1 commit into
nadbm:masterfrom
patrick-jones:escape
Jun 1, 2017
Merged

Adds the ability to revert an edit by hitting the escape key.#22
nadbm merged 1 commit into
nadbm:masterfrom
patrick-jones:escape

Conversation

@patrick-jones
Copy link
Copy Markdown
Contributor

Closes #21

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.04%) to 98.661% when pulling 56ffb3d on patrick-jones:escape into 95ebb2c on nadbm:master.

@nadbm
Copy link
Copy Markdown
Owner

nadbm commented Jun 1, 2017

hey @patrick-jones , no need to add a new state for this. Check the method "pageClick" , which essentially does what you're describing. Use a similar function (this.setState(this.defaultState)) when the ESC button is clicked

@patrick-jones
Copy link
Copy Markdown
Contributor Author

@nadbm , that doesn't seem to revert the cell to the original value. componentDidUpdate in DataCell will still fire onChange when you exit edit mode unless you have an additional property to tell it not to publish the change.

@nadbm
Copy link
Copy Markdown
Owner

nadbm commented Jun 1, 2017

Yep my mistake, you're right. Can you add a test? Thanks for the PR 💯

@patrick-jones
Copy link
Copy Markdown
Contributor Author

There's a test in there, line 669 of Datasheet.js.

By the way, I really like the approach you've taken with this grid. Other grids are all to monolithic, baking too much directly in the grid itself instead of being model-driven.

I actually need a good grid for a work project I'm doing. Do you mind if a take a swipe at adding a few features such as dragging/dropping rows, resizable columns?

@nadbm
Copy link
Copy Markdown
Owner

nadbm commented Jun 1, 2017

Right, git collapsed a lot of it so did not see the test. I'll merge this now.

And for those features, go right ahead. I wanted to only keep UX logic in this component, everything else can be handled by the user. So drag/drop and resizable columns definitely fits right in.

@nadbm nadbm merged commit a3f0a04 into nadbm:master Jun 1, 2017
@patrick-jones patrick-jones deleted the escape branch June 1, 2017 19:12
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.

3 participants