-
Notifications
You must be signed in to change notification settings - Fork 25
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
fix: avoid unnecessary initial filter loading state #2503
Conversation
- use all manual attributions as filtered attributions when no filter is set - separate concerns of signals worker hook from those of the hooks accessing variables from the Redux store closes #2502 Signed-off-by: Maxim Stykow <maxim.stykow@tngtech.com>
5f1bb8c
to
26144b0
Compare
|
||
export const FOLDER_PROGRESS_DATA = 'folder-progress-data'; | ||
|
||
export function useFolderProgressData() { |
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 could also have the simple hooks accumulated in one file, but I think it is nice to have them separated like you did it.
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.
yes, i was considering along the same lines
import { getManualAttributions } from '../selectors/all-views-resource-selectors'; | ||
import { useVariable } from './use-variable'; | ||
|
||
export const FILTERED_ATTRIBUTIONS = 'filtered-attributions'; |
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 always do that 👍
I was thinking we could add _KEY
to these variable names to make their purpose clear.
setFilteredAttributions((prev) => ({ | ||
...prev, | ||
attributions, | ||
})); |
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.
Why do we pass an arrow function here instead of just passing attributions
?
setFilteredAttributions((prev) => ({ | |
...prev, | |
attributions, | |
})); | |
setFilteredAttributions( | |
attributions, | |
); |
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.
because i need to preserve the other attributes that are inside prev
but not part of attributions
}); | ||
}); | ||
|
||
it('sends selected filters when some are set', () => { |
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.
👍
useFilteredAttributions, | ||
} from '../use-filtered-attributions'; | ||
|
||
describe('useFilteredAttributions', () => { |
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.
👍
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.
Looks good to me and works as expected.
For a more pleasant review, I would have preferred to have the refactorings in a separate pull request, though.
Summary of changes
Context and reason for change
closes #2502
How can the changes be tested
Open a large Opossum file and notice that the attribution list immediately displays attributions without any filter loading spinner spinning.