Skip to content
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

🐛 Broken search string when navigating from deps page #1662

Merged
merged 4 commits into from
Jan 19, 2024

Conversation

ibolton336
Copy link
Member

@ibolton336 ibolton336 commented Jan 11, 2024

Resolves [Dependencies]Application inventory page shows no applications once Accessed from dependencies
Resolves https://issues.redhat.com/browse/MTA-2056

  • Removes the auto applied search string when direct navigating using the side nav. This was applying filters that were not registered within the application table ( & other tables). I.E when navigating from the dependencies page, the search string was including an "applications.name" filter which is just referenced as "name" within the applications table. Future iterations may allow us to rewrite the search string when navigating between pages or standardizing on a specific key for app names on the back end.
  • Adds the ability to parse the url string for initial search values in the dependencies toolbar.
  • Adds associated clear filters handler to clear the url when the toolbar clear button is pressed. This is necessary to overwrite the initial filter values. Implemented this in dependencies page & applications table. There may be a need to incorporate this into filter toolbar at some point.

@ibolton336 ibolton336 added the cherry-pick/release-0.3 This PR should be cherry-picked to release-0.3 branch. label Jan 11, 2024
Copy link
Member

@sjd78 sjd78 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes LGTM.

Does the "clear all" function need to be applied to more tables? Should it exist in all filterable tables? If yes, either this PR can be expanded, or a followup issue needs to be created.

@ibolton336
Copy link
Member Author

ibolton336 commented Jan 18, 2024

The changes LGTM.

Does the "clear all" function need to be applied to more tables? Should it exist in all filterable tables? If yes, either this PR can be expanded, or a followup issue needs to be created.

This one is tough because the toolbar is a pf library component which manages the chip state and the clear button state. The toolbar exposes the clearAllFilters callback for special cases like this one when there are initial filter values parsed and loaded from the url query params. Ideally I'd like to move this code into our custom filter toolbar component based on the presence of initialFilterValues, but that doesn't seem possible at the moment.

This PR does cover both of the cases where we are using this initialFilterValue set logic, though. It is a bit annoying that we will have to remember this for any of these cases going forward.

@sjd78
Copy link
Member

sjd78 commented Jan 19, 2024

The changes LGTM.
Does the "clear all" function need to be applied to more tables? Should it exist in all filterable tables? If yes, either this PR can be expanded, or a followup issue needs to be created.

This one is tough because the toolbar is a pf library component which manages the chip state and the clear button state. The toolbar exposes the clearAllFilters callback for special cases like this one when there are initial filter values parsed and loaded from the url query params. Ideally I'd like to move this code into our custom filter toolbar component based on the presence of initialFilterValues, but that doesn't seem possible at the moment.

Yeah that makes this one a bit rough for sure.

This PR does cover both of the cases where we are using this initialFilterValue set logic, though. It is a bit annoying that we will have to remember this for any of these cases going forward.

Makes sense. Probably best to open an issue to log this future work and make it easy to find if/when the search string stuff comes up again.

Copy link
Member

@sjd78 sjd78 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just needs the issue generated so we don't forget the solution if/when it comes up again on another table.

@ibolton336
Copy link
Member Author

#1667

@ibolton336 ibolton336 merged commit 8f83736 into konveyor:main Jan 19, 2024
6 checks passed
github-actions bot pushed a commit that referenced this pull request Jan 19, 2024
Resolves [[Dependencies]Application inventory page shows no applications
once Accessed from
dependencies](https://issues.redhat.com/browse/MTA-2007)
Resolves https://issues.redhat.com/browse/MTA-2056

- Removes the auto applied search string when direct navigating using
the side nav. This was applying filters that were not registered within
the application table ( & other tables). I.E when navigating from the
dependencies page, the search string was including an
"applications.name" filter which is just referenced as "name" within the
applications table. Future iterations may allow us to rewrite the search
string when navigating between pages or standardizing on a specific key
for app names on the back end.
- Adds the ability to parse the url string for initial search values in
the dependencies toolbar.
- Adds associated clear filters handler to clear the url when the
toolbar clear button is pressed. This is necessary to overwrite the
initial filter values. Implemented this in dependencies page &
applications table. There may be a need to incorporate this into filter
toolbar at some point.

Signed-off-by: ibolton336 <[email protected]>
Co-authored-by: Scott Dickerson <[email protected]>
ibolton336 added a commit that referenced this pull request Jan 19, 2024
Resolves [[Dependencies]Application inventory page shows no applications
once Accessed from
dependencies](https://issues.redhat.com/browse/MTA-2007)
Resolves https://issues.redhat.com/browse/MTA-2056

- Removes the auto applied search string when direct navigating using
the side nav. This was applying filters that were not registered within
the application table ( & other tables). I.E when navigating from the
dependencies page, the search string was including an
"applications.name" filter which is just referenced as "name" within the
applications table. Future iterations may allow us to rewrite the search
string when navigating between pages or standardizing on a specific key
for app names on the back end.
- Adds the ability to parse the url string for initial search values in
the dependencies toolbar.
- Adds associated clear filters handler to clear the url when the
toolbar clear button is pressed. This is necessary to overwrite the
initial filter values. Implemented this in dependencies page &
applications table. There may be a need to incorporate this into filter
toolbar at some point.

Signed-off-by: ibolton336 <[email protected]>
Co-authored-by: Scott Dickerson <[email protected]>
Signed-off-by: ibolton336 <[email protected]>
ibolton336 added a commit that referenced this pull request Feb 12, 2024
Resolves: https://issues.redhat.com/browse/MTA-2007
Resolves: https://issues.redhat.com/browse/MTA-2056
Backport of #1662 

- Removes the auto applied search string when direct navigating using
the side nav. This was applying filters that were not registered within
the application table ( & other tables). I.E when navigating from the
dependencies page, the search string was including an
"applications.name" filter which is just referenced as "name" within the
applications table. Future iterations may allow us to rewrite the search
string when navigating between pages or standardizing on a specific key
for app names on the back end.
- Adds the ability to parse the url string for initial search values in
the dependencies toolbar.
- Adds associated clear filters handler to clear the url when the
toolbar clear button is pressed. This is necessary to overwrite the
initial filter values. Implemented this in dependencies page &
applications table. There may be a need to incorporate this into filter
toolbar at some point.

Signed-off-by: ibolton336 <[email protected]>
Co-authored-by: Scott Dickerson <[email protected]>

Signed-off-by: ibolton336 <[email protected]>
Co-authored-by: Ian Bolton <[email protected]>
Co-authored-by: Scott Dickerson <[email protected]>
ibolton336 added a commit that referenced this pull request Feb 12, 2024
Resolves: https://issues.redhat.com/browse/MTA-2007
Resolves: https://issues.redhat.com/browse/MTA-2056
Backport of #1662 

- Removes the auto applied search string when direct navigating using
the side nav. This was applying filters that were not registered within
the application table ( & other tables). I.E when navigating from the
dependencies page, the search string was including an
"applications.name" filter which is just referenced as "name" within the
applications table. Future iterations may allow us to rewrite the search
string when navigating between pages or standardizing on a specific key
for app names on the back end.
- Adds the ability to parse the url string for initial search values in
the dependencies toolbar.
- Adds associated clear filters handler to clear the url when the
toolbar clear button is pressed. This is necessary to overwrite the
initial filter values. Implemented this in dependencies page &
applications table. There may be a need to incorporate this into filter
toolbar at some point.

Signed-off-by: ibolton336 <[email protected]>
Co-authored-by: Scott Dickerson <[email protected]>

Signed-off-by: ibolton336 <[email protected]>
Co-authored-by: Ian Bolton <[email protected]>
Co-authored-by: Scott Dickerson <[email protected]>
ibolton336 added a commit that referenced this pull request Feb 12, 2024
Resolves: https://issues.redhat.com/browse/MTA-2007
Resolves: https://issues.redhat.com/browse/MTA-2056
Backport of #1662

- Removes the auto applied search string when direct navigating using
the side nav. This was applying filters that were not registered within
the application table ( & other tables). I.E when navigating from the
dependencies page, the search string was including an
"applications.name" filter which is just referenced as "name" within the
applications table. Future iterations may allow us to rewrite the search
string when navigating between pages or standardizing on a specific key
for app names on the back end.
- Adds the ability to parse the url string for initial search values in
the dependencies toolbar.
- Adds associated clear filters handler to clear the url when the
toolbar clear button is pressed. This is necessary to overwrite the
initial filter values. Implemented this in dependencies page &
applications table. There may be a need to incorporate this into filter
toolbar at some point.

Signed-off-by: ibolton336 <[email protected]>
Co-authored-by: Scott Dickerson <[email protected]>

Signed-off-by: ibolton336 <[email protected]>
Co-authored-by: Ian Bolton <[email protected]>
Co-authored-by: Scott Dickerson <[email protected]>
Signed-off-by: ibolton336 <[email protected]>
@ibolton336 ibolton336 deleted the mta-2007 branch February 26, 2024 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick/release-0.3 This PR should be cherry-picked to release-0.3 branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants