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(ui): Fix node filters in UI #5162
Conversation
Signed-off-by: Simon Behar <simbeh7@gmail.com>
(!nodeClassNames || Object.entries(nodeClassNames).find(([className, checked]) => checked && (label.classNames || '').split(' ').includes(className))) && | ||
(!nodeTags || Object.entries(nodeTags).find(([tag, checked]) => !label.tags || (checked && label.tags.has(tag)))) && | ||
!nodeSearchKeyword) || | ||
label.label.includes(nodeSearchKeyword) |
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.
This line broke the filter. If nodeSearchKeyword
was empty it would match all nodes, rendering the filter moot
return true; | ||
} | ||
// If the node doesn't match enabled genres, don't display | ||
if (!nodeGenres[label.genre]) { |
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 we add some tests?
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 spent some time trying, but it's a pain: will need to extract the function out of the component, create parameters for props
and all the required variables, and then mock them in the test. I'd be happy to do so, but don't think it's worth it. Thoughts?
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.
sounds like poor roi, leave 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.
@alexec Ready for review then
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.
LGTM. I think it is easier to understand too, by removing the large expression.
Checklist: