-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Make rollup group behaviour a setting in the global settings menu #2183
Conversation
@mofojed I tried experimenting with trying to have the I am going to push the "hacky" code that we discussed yesterday, and then we can continue the conversation on finding a cleaner way to do this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will investigate unit test failures
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2183 +/- ##
========================================
Coverage 46.64% 46.64%
========================================
Files 692 693 +1
Lines 38571 38606 +35
Branches 9647 9839 +192
========================================
+ Hits 17992 18009 +17
+ Misses 20568 20544 -24
- Partials 11 53 +42
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
For the image mismatches, will need to have new images taken, to replace |
@@ -71,6 +71,10 @@ const IrisGridModelUpdater = React.memo( | |||
columnHeaderGroups, | |||
partitionConfig, | |||
}: IrisGridModelUpdaterProps) => { | |||
if (model.formatter !== formatter) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the "less" hacky way to fix this is to get rid of all the useEffect
s in here. Those run after the render, when really all we want from this component is to set values when they have changed. And then make sure it checks on every render (i.e. the component shouldn't be memoized at the top), and then we don't need to pass in a modelColumns
prop. Instead could make a hook to run immediately when something has changed, e.g.
function useOnChange(callback: () => void, deps: DependencyList): void {
// Want to call the callback if the deps have changed from the previous time
const prevDeps = usePrevious(deps);
if (prevDeps === undefined || !deps.every((dep, i) => dep === prevDeps[i])) {
callback();
}
}
Then instead of useEffect
in IrisGridModelUpdater
, just use useOnChange
, and use model.columns
instead of modelColumns
, get rid of the modelColumns
prop, and don't wrap the function in React.memo
. I think that will work.
@mofojed Adjusted logic according to your review, e2e tests may still fail because of the rollup images. How can we update those? |
@AkshatJawne Run |
@mofojed Ran command, appears the snapshots were not updated? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ready for re-review @mofojed @mattrunyon
...s/table-operations.spec.ts-snapshots/rollup-rows-and-aggregrate-columns-1-chromium-linux.png
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to fix test case
Closes #2128