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

Add filtering through search params #908

Merged

Conversation

rtorrero
Copy link
Contributor

@rtorrero rtorrero commented Oct 18, 2022

Description

Peek 18-10-2022 16-02

This PR was initially supposed to just add linking from the dashboard to the hosts view with filters set in the search params, but after starting, it was clear that by allowing the filter component to handle the search params we could enable it in every view.

@CDimonaco actually has another approach on the works, if that works/looks better we can just close this, I just didn't want so many hours of bashing my head against the keyboard to end in nothing 😄

How was this tested?

A new e2e test was added to check that the link from the dashboard includes the filter preset.

@rtorrero rtorrero added enhancement New feature or request javascript Pull requests that update Javascript code proposal labels Oct 18, 2022
@rtorrero rtorrero force-pushed the add-searchparams-filtering-hosts branch from 056faea to f1e3873 Compare October 18, 2022 14:38
@rtorrero rtorrero marked this pull request as ready for review October 18, 2022 15:04
Copy link
Member

@CDimonaco CDimonaco left a comment

Choose a reason for hiding this comment

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

I think we should investigate and remove the JSON stringify stuff and refactor the flow in a top down fashion

@rtorrero rtorrero force-pushed the add-searchparams-filtering-hosts branch from 4dd6151 to a2bee8d Compare October 21, 2022 16:29
@rtorrero
Copy link
Contributor Author

Since PR #925 has already been merged and included improvements over the work that I did here, I've removed one of my commits and rebased. Should be good now.

Copy link
Contributor

@dottorblaster dottorblaster left a comment

Choose a reason for hiding this comment

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

LGTM

@rtorrero rtorrero merged commit db379d9 into trento-project:main Oct 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request javascript Pull requests that update Javascript code proposal
Development

Successfully merging this pull request may close these issues.

4 participants