Open YuliaKrimerman opened 4 days ago
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please assign manosnoam for approval. For more information see the Kubernetes Code Review Process.
The full list of commands accepted by this bot can be found here.
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 84.84%. Comparing base (
330dfdf
) to head (454e81f
). Report is 9 commits behind head on main.
@YuliaKrimerman I think we need to fix this issue in Deployed models page as well.
@ppadti I was trying to add that fix into InferenceServiceTable inside the Table, but even though it has emptyTableView that whole chip group is not showing, what am I missing?
@ppadti I don't think we have a toolbar with filters for the deployments tab here as per the mocks. Let me know if you're referring to some other mock links.
Yeah, I was referring to this slack conversation here.
@ppadti , @manaswinidas As I mentioned above I was trying to add that fix into InferenceServiceTable(it's the right file correct?) inside the Table, but even though it has emptyTableView that whole chip group is not showing, what am I missing? Any suggestions? And yes, I suggested to fix it as part of that PR in that conversation, but just got stuck, and preferred to, at least put the required solution for the actual bug
@ppadti , @manaswinidas As I mentioned above I was trying to add that fix into InferenceServiceTable(it's the right file correct?) inside the Table, but even though it has emptyTableView that whole chip group is not showing, what am I missing? Any suggestions? And yes, I suggested to fix it as part of that PR in that conversation, but just got stuck, and preferred to, at least put the required solution for the actual bug
May be because it is using DashboardSearchField and not ToolbarFilter and we have this issue in some other places as well for ex: Accelerator profile and Notebook images page. I think we need to discuss this with the team and can have a separate issue to address this, if needed. wdyt?
Sounds good to me, I can open an issue for it, and will address the related PR comments then.
@manaswinidas Addressed comment, ready for re-review. Also created a story for us to discuss https://issues.redhat.com/browse/RHOAIENG-13924 . @ppadti . Thank you!
https://issues.redhat.com/browse/RHOAIENG-12861
Description
How Has This Been Tested?
on the UI
Test Impact
Updated tests to pass after the label change. In order to test the change, verify that now "Clear all Filters' button is now working everywhere it exists
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main