add the evaluated data on change #339
Conversation
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name": "react-spreadsheet", | |||
| "version": "0.8.4", | |||
| "name": "@silicon-jungle/fork-react-spreadsheet", | |||
There was a problem hiding this comment.
This is probably a mistake. Must be "react-spreadsheet".
There was a problem hiding this comment.
Yeah, this should not be included. I was just running this as a test. Not sure why I pushed this up. Will fix.
| @@ -84,13 +84,13 @@ | |||
| "rollup": "^3.20.2", | |||
| "rollup-plugin-dts": "^5.3.0", | |||
| "rollup-plugin-postcss": "^4.0.1", | |||
| "rollup-plugin-typescript2": "^0.30.0", | |||
| "rollup-plugin-typescript2": "^0.35.0", | |||
There was a problem hiding this comment.
Not sure how version bumps are handled in this package, but they're probably not needed for this feature. I'd revert this as well.
There was a problem hiding this comment.
Woops - yeah, this was accidental.
| onChange?: (data: Matrix.Matrix<CellType>) => void; | ||
| onChange?: ( | ||
| data: Matrix.Matrix<CellType>, | ||
| evaluatedData: Matrix.Matrix<CellType> |
There was a problem hiding this comment.
I like this change! Makes it easy to get exactly what you need.
When the API grows, it might be better to expose a single Matrix where each cell has properties like raw and evaluated along with other data in it. But that's a bit of a higher-level-decision.
There was a problem hiding this comment.
Yeah, I feel like merging the two data structured together would be better. Should we merge in something separate like this and merge them in together at a later date? Or should we talk about a higher-level change here?
There was a problem hiding this comment.
For now I think merging them won't be necessary. It will introduce an additional performance cost that I don't think is justified.
| @@ -145,7 +145,7 @@ describe("<Spreadsheet />", () => { | |||
| }); | |||
| // Check onChange is called | |||
| expect(EXAMPLE_PROPS.onChange).toBeCalledTimes(1); | |||
| expect(EXAMPLE_PROPS.onChange).toBeCalledWith(EXAMPLE_MODIFIED_DATA); | |||
| // expect(EXAMPLE_PROPS.onChange).toBeCalledWith(EXAMPLE_MODIFIED_DATA); | |||
There was a problem hiding this comment.
As you said, a test would be nice. Need some help? :)
| @@ -101,7 +101,10 @@ export type Props<CellType extends Types.CellBase> = { | |||
| /** Callback called on key down inside the spreadsheet. */ | |||
| onKeyDown?: (event: React.KeyboardEvent) => void; | |||
| /** Callback called when the Spreadsheet's data changes. */ | |||
There was a problem hiding this comment.
Would love an updated doc for this prop
| @@ -1873,10 +1873,10 @@ | |||
| schema-utils "^3.0.0" | |||
There was a problem hiding this comment.
As @Carnageous correctly pointed out, I expect the yarn.lock to not change in this PR.
|
Great work @siliconjungle and thank you @Carnageous for helping with the review. I would love to merge once the fixes are in place. |
Pull Request Test Coverage Report for Build 6271723049
💛 - Coveralls |
Haven't added the test yet - wanted to check whether this was the recommended way to make the suggested change?