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

Change instance filtering to url instead of state [WD-7026] #527

Merged
merged 4 commits into from
Nov 14, 2023

Conversation

rubinaga
Copy link
Contributor

@rubinaga rubinaga commented Nov 9, 2023

Done

  • Changed instance filtering to use url search query parameters instead of state
  • Changed eslintrc to ignore variables that start with underscore as they remain unused ( happens in the case of useSearchParams where the variable is not used in the file InstanceSearchFilter.tsx)

QA

  1. Run the LXD-UI:
    • On the demo server via the link posted by @webteam-app below. This is only available for PRs created by collaborators of the repo. Ask @lorumic or @edlerd for access.
    • With a local copy of this branch, run as described here.
  2. Perform the following QA steps:
    • Go to instances page
    • Apply any combination of filters
    • Check if they are applied correctly
    • Check if filters are applied when reloading the page or pasting an url with existing filters on it
    • Check if additional filters can be applied/removed in the case mentioned above

@webteam-app
Copy link

Demo starting at https://lxd-ui-527.demos.haus

@rubinaga rubinaga changed the title Change instance filtering to url [WD-7026] Change instance filtering to url instead of state [WD-7026] Nov 9, 2023
Copy link
Contributor

@lorumic lorumic left a comment

Choose a reason for hiding this comment

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

Very well done :) I've left a few comments.

src/pages/instances/InstanceSearchFilter.tsx Outdated Show resolved Hide resolved
src/pages/instances/InstanceList.tsx Outdated Show resolved Hide resolved
src/pages/instances/InstanceSearchFilter.tsx Outdated Show resolved Hide resolved
src/pages/instances/InstanceSearchFilter.tsx Outdated Show resolved Hide resolved
src/pages/instances/InstanceSearchFilter.tsx Outdated Show resolved Hide resolved
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

This works really well, good work!
Some ideas to simplify the code in addition to @lorumic below.

src/pages/instances/InstanceList.tsx Outdated Show resolved Hide resolved
src/pages/instances/InstanceSearchFilter.tsx Outdated Show resolved Hide resolved
@rubinaga
Copy link
Contributor Author

  • Changed back the eslint file.
  • Shortened the url parameters
  • Refactored comparison from complete url to only search parameters
  • Removed comment
  • Removed unnecessary enrich of status

@rubinaga
Copy link
Contributor Author

  • Fixed wrong type being applied on reload ( David's solution )

Copy link
Contributor

@lorumic lorumic left a comment

Choose a reason for hiding this comment

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

It all looks good and works as expected. I have left a comment, but it's a minor thing. Feel free to implement it or not.

if (url.href !== window.location.href) {
setSearchParams(url.searchParams);
if (newSearchParams.toString() !== currentWindowSearchParams.toString()) {
setSearchParams(newSearchParams);
}
};

const getChips = (): SearchAndFilterChip[] => {
const searchParams = new URLSearchParams(window.location.search);
const statusChipUrl = [
Copy link
Contributor

Choose a reason for hiding this comment

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

I just realised that this now looks pretty similar to the items that we have below, inlined with the newSearchData array. I wonder if this should be inlined there too, instead of having, it alone, its own const declaration?

Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for applying the changes :)

@rubinaga rubinaga merged commit d66601c into canonical:main Nov 14, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants