-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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 search toolbar clear all filters #11939
Fix search toolbar clear all filters #11939
Conversation
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 did a cursory look at this and found some things that we could improve on. If there are cases where my requested changes wouldn't work, like were searchChips is an array and not an object, then I'm good with the code as is.
const searchChips = getChipsByKey(params, columns, qsConfig); | ||
const [chipsByKey, setChipsByKey] = useState( | ||
JSON.parse(JSON.stringify(searchChips)) | ||
); |
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.
const searchChips = getChipsByKey(params, columns, qsConfig); | |
const [chipsByKey, setChipsByKey] = useState( | |
JSON.parse(JSON.stringify(searchChips)) | |
); | |
const [chipsByKey, setChipsByKey] = useState( | |
getChipsByKey(params, columns, qsConfig) | |
); |
useEffect(() => { | ||
Object.keys(chipsByKey).forEach((el) => { | ||
chipsByKey[el].chips = []; | ||
}); | ||
setChipsByKey({ ...chipsByKey, ...searchChips }); | ||
}, [location.search, searchChips.length]); // eslint-disable-line react-hooks/exhaustive-deps |
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.
useEffect(() => { | |
Object.keys(chipsByKey).forEach((el) => { | |
chipsByKey[el].chips = []; | |
}); | |
setChipsByKey({ ...chipsByKey, ...searchChips }); | |
}, [location.search, searchChips.length]); // eslint-disable-line react-hooks/exhaustive-deps | |
useEffect(() => { | |
Object.keys(chipsByKey).forEach((el) => { | |
chipsByKey[el].chips = []; | |
}); | |
setChipsByKey(getChipsByKey(params, columns, qsConfig)); | |
}, [location.search]); |
searchChips is an object so it looks like searchChips.length is always going to be undefined. if we can remove that dependency from the array then I think we can also remove the commented eslint-disable-line...
Finally, since we want to keep chipsByKey in sync with searchChips lets just use the getChipsByKey function to give us those values here.
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 agree that searchChips.length
is unnecessary here and should be removed from the dependency array since it is a js object.
const [chipsByKey, setChipsByKey] = useState(
JSON.parse(JSON.stringify(searchChips))
);
Initializing the chipsByKey
state (above) is key to removing any object reference to searchChips
and to persisting the original filters. The goal of this PR was to build up the list of filters and maintain them even after the params change. When the user deletes a filter, instead of removing it from the chipsByKey
object, we pass that specific filter's chips property an empty array. Keeping the original filter key around for PatternFly's Toolbar component was the trick to fixing the bug.
setChipsByKey(getChipsByKey(params, columns, qsConfig));
The line above would overwrite the list of filters and bring back the original bug.
setChipsByKey({ ...chipsByKey, ...searchChips });
By spreading the original chipsByKey
and the current searchChips
into the setter, I'm able to update the state with new search filters from searchChips
while preserving the older search filter keys.
5f70410
to
dc07a7a
Compare
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 talked offline. We are going to make patternfly aware of their toolbar dynamic filtering of the toolbar. This code works well for us. Good work!
dc07a7a
to
ac604f6
Compare
ac604f6
to
1de2487
Compare
SUMMARY
Issue: #8474
Dynamically removing the ToolbarFilter component threw the Toolbar component’s inner
filterInfo
state out of sync. The filters were responsible for notifying the Toolbar’s context of changes, so by removing them prematurely, the Toolbar was unaware that it needed to remove filters from its state. This PR persists the ToolbarFilter components, which allows the Toolbar to manage showing and hiding the filter list.ISSUE TYPE
COMPONENT NAME