-
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: DH-14630 - ACL Editor Hooks #1257
Conversation
82d1f74
to
ead50c4
Compare
e099c9a
to
7ce7ea0
Compare
511b47f
to
21d7b82
Compare
Codecov Report
@@ Coverage Diff @@
## main #1257 +/- ##
==========================================
+ Coverage 45.57% 45.76% +0.18%
==========================================
Files 484 490 +6
Lines 34077 34202 +125
Branches 8532 8554 +22
==========================================
+ Hits 15531 15652 +121
- Misses 18495 18499 +4
Partials 51 51
Flags with carried forward coverage won't be shown. Click here to find out more.
|
* React hook that closes a given table when the component unmounts. | ||
* @param table | ||
*/ | ||
export default function useTableCloseOnUnmount( |
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.
Being pedantic, this would close the table on unmount OR if the table changes. I can't think of a better name though and don't feel too strongly about it.
packages/jsapi-components/src/useDebouncedViewportSearch.test.ts
Outdated
Show resolved
Hide resolved
expect(TableUtils.makeFilterValue).not.toHaveBeenCalled(); | ||
expect(viewportData.applyFiltersAndRefresh).toHaveBeenCalledWith([]); | ||
} | ||
); |
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'd add another test:
it('should cancel debounce on unmount', () => {
const searchText = 'mock.searchText';
const debounceMs = 400;
const { result, unmount } = renderHook(() =>
useDebouncedViewportSearch(viewportData, 'mock.column', debounceMs)
);
result.current(searchText);
jest.advanceTimersByTime(5);
unmount();
jest.advanceTimersByTime(debounceMs);
expect(TableUtils.makeFilterValue).not.toHaveBeenCalled();
expect(viewportData.applyFiltersAndRefresh).not.toHaveBeenCalled();
});
): (searchText: string) => void { | ||
return useMemo( | ||
() => | ||
debounce((searchText: string) => { |
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.
We should be cancelling the debounce on unmount.
expect(maybeTable.close).not.toHaveBeenCalled(); | ||
} | ||
} | ||
); |
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.
Should add a case that it closes the old table if the table provided changes.
|
||
rerender(nextTable); | ||
|
||
expect(table.close).toHaveBeenCalled(); |
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.
expect(table.close).toHaveBeenCalled(); | |
expect(table.close).toHaveBeenCalled(); | |
expect(nextTable.close).not.toHaveBeenCalled(); |
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.
Fixed and pushed
Supports enterprise DH-14630
closes #1260
BREAKING CHANGE:
generateEmptyKeyedItemsRange
previously required a singlecount
arg, but now requires astart
andend
index