-
Notifications
You must be signed in to change notification settings - Fork 163
Add filters to node map #580
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Nice change, looking forward to using it. Overall its a lot of good work, but I'd like to see some improvements made to the type casting (prefer type assertion functions instead), array index key usage, and ! operator in cfg.bounds.
const nodeNum = node.num?.toString() ?? ""; | ||
const nodeNumHex = numberToHexUnpadded(node.num) ?? ""; | ||
const search = text.toLowerCase(); | ||
return shortName.includes(search) || longName.includes(search) || |
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 doesn't need to change, but in the future you could have done return [shortName, longName, nodeNum, nodeNumHex].some(s => s.includes(search).
It helps avoid the all of the OR statements.
Overall I really like this feature. The one improvement I would suggest we make before merging is adding some way of indicating you've applied a filter. Either by changing the icon filter icon (swapping to a filled icon if any filters are configured), or adding a shadow or text to indicate filters are active. What do you think @philon- ? |
Good suggestion! Maybe something like this? ![]() |
Description
Added some filtering options to the node map. Now users can filter by text (
longName
/shortName
/num
), number of hops, channel & air utilization and battery level.The filtering logic is abstracted into
util/hooks/useNodeFilters.ts
for possible future implementation into the node list page.Extra children/objects can be added onto the UI popover if there is a need to extend with other map UI elements (re PR #457)
Changes Made
util/hooks/useNodeFilters.ts
which handles filtering logic, including definition and criteria.Slider
fromradix-ui/react-slider
FilterControl
object which uses a popover object to show input/form elements based on filters defined in the hook.components/UI/Checkbox/index.tsx
in order to make sure thatsetIsChecked
state updates when filters are reset.Testing Done
Tested in Safari/Chrome/Firefox on macOS.
Checkbox element still passes tests after changes
Linting with
deno lint
passes (for changed/added files)I didn't have any MQTT-nodes so that filter is currently untested.
Screenshots (if applicable)
Checklist
Additional Notes
My react/typescript knowledge is pretty rusty so I'm open for any feedback on this 😉