-
Notifications
You must be signed in to change notification settings - Fork 772
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(cyclops-ui): Filter Modules by TargetNamespace #666
base: main
Are you sure you want to change the base?
feat(cyclops-ui): Filter Modules by TargetNamespace #666
Conversation
@quest-bot loot #640 |
hey @petar-cvit @KaradzaJuraj, can you please review this pr in your free time? |
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.
Hey @s-vamshi, the filter resets every 10 seconds because we refresh the module list every 10 seconds.
Use the /api/namespaces
endpoint to populate the filter data even before you get the modules.
@@ -24,18 +24,40 @@ import { mapResponseError } from "../../../utils/api/errors"; | |||
|
|||
const { Title } = Typography; | |||
|
|||
interface ModuleInterface { |
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 remove this for now, we'd have to implement it in more places. Let's keep the scope of the PR to just what was defined in the issue.
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.
sure will remove it!
const Modules = () => { | ||
const [allData, setAllData] = useState([]); | ||
const [filteredData, setFilteredData] = useState([]); | ||
const [allData, setAllData] = useState<ModuleInterface[]>([]); |
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.
Revert to empty array
const [allData, setAllData] = useState([]); | ||
const [filteredData, setFilteredData] = useState([]); | ||
const [allData, setAllData] = useState<ModuleInterface[]>([]); | ||
const [filteredData, setFilteredData] = useState<ModuleInterface[]>([]); |
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.
Revert to empty array
return self.indexOf(targetNamespace) === index; | ||
}); | ||
setNamespaceFilterData(namespaceData); | ||
setModuleNamespaceFilter(namespaceData); |
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 cannot use setModuleNamespaceFilter
here because it is called every 10 seconds and resets the filter.
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.
true didnt check this my bad, will update
@@ -49,6 +71,7 @@ const Modules = () => { | |||
.get(`/api/modules/list`) | |||
.then((res) => { | |||
setAllData(res.data); | |||
populateNamespaceData(res.data); |
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.
Move the call of this function outside the fetchModules
loop.
In this useEffect, before the fetchModules
function, create a call towards /api/namespaces
. This will return all available namespaces with which you can populate both the namespaceFilterData
and the moduleNamespaceFilter
hey @KaradzaJuraj thanks for reviewing this, will be pushing the changes soon! |
hey @KaradzaJuraj made the changes as mentioned can you please review them in your free time? |
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.
Hey @s-vamshi, just a few minor tweaks and it should be good to go!
hey @KaradzaJuraj, done with the changes! please review them in your free time!! |
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.
Thanks @s-vamshi, looks great! 🚀
closes #640
📑 Description
✅ Checks
ℹ Additional context
screen-capture.1.webm