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

Search: Do not change route path when clearing fuzzy search #804

Merged
merged 4 commits into from
Feb 29, 2024

Conversation

tkohr
Copy link
Collaborator

@tkohr tkohr commented Feb 23, 2024

Description

Currently, the router navigates to the search when the fuzzy search entry is cleared (with the input's cross) on a different tab/page than the search. This PR intends to maintain the same path when clearing the search. The fuzzy-search.component now only calls updateFilters if searchFilters.any is not empty or undefined.

Also updates @geospatial-sdk/geocoding for the npm package and attempts to fix a flaky e2e test.

Quality Assurance Checklist

  • Commit history is devoid of any merge commits and readable to facilitate reviews
  • If new logic ⚙️ is introduced: unit tests were added
  • If new user stories 🤏 are introduced: E2E tests were added
  • If new UI components 🕹️ are introduced: corresponding stories in Storybook were created
  • If breaking changes 🪚 are introduced: add the breaking change label
  • If bugs 🐞 are fixed: add the backport <release branch> label
  • The documentation website 📚 has received the love it deserves

This work is sponsored by MEL.

Copy link
Contributor

github-actions bot commented Feb 23, 2024

Affected libs: feature-search, feature-router, feature-map, feature-dataviz, feature-record,
Affected apps: metadata-editor, datahub, demo, webcomponents, search, map-viewer,

  • 🚀 Build and deploy storybook and demo on GitHub Pages
  • 📦 Build and push affected docker images

@tkohr tkohr marked this pull request as ready for review February 28, 2024 10:22
@coveralls
Copy link

coveralls commented Feb 28, 2024

Coverage Status

coverage: 86.281% (+2.8%) from 83.444%
when pulling d94ca4a on fuzzy-search-clear
into 5b8d393 on main.

Copy link
Collaborator

@jahow jahow left a comment

Choose a reason for hiding this comment

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

Thanks for all the added tests! I did a suggestion to make the code more straightforward, what do you think?

@@ -76,6 +79,8 @@ export class FuzzySearchComponent implements OnInit {
}

handleInputCleared() {
this.searchService.updateFilters({ any: '' })
if (this.currentSearchFilters.any) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of storing an internal state here you can do something like so:

async handleInputCleared() {
  const currentSearchFilters = await firstValueFrom(this.searchFacade.searchFilters$);
  if (!currentSearchFilters.any) { return; }
  this.searchService.updateFilters({ any: '' })
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice idea, also works as expected, thanks! I just inverted the if to save another line.

Copy link
Collaborator

@Angi-Kinas Angi-Kinas left a comment

Choose a reason for hiding this comment

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

Thanks @tkohr for changing the behavior for clearing the search input! It looks good to me 👍

@tkohr
Copy link
Collaborator Author

tkohr commented Feb 29, 2024

Thanks for the reviews! Unfortunately, the e2e test I tried to improve still does not pass systematically and needed a re-run.

@tkohr tkohr merged commit a9d3352 into main Feb 29, 2024
9 checks passed
@tkohr tkohr deleted the fuzzy-search-clear branch February 29, 2024 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants