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

Skipping searchbar preview on reload #10232

Merged
merged 5 commits into from
Jan 8, 2024

Conversation

jacob-nv
Copy link
Contributor

Description

Setting "search term" from route would trigger search, which would create request on page load.
Added additional bool value to check if this change is triggered by route or user input

Related Issue

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@jacob-nv jacob-nv self-assigned this Dec 21, 2023
Copy link

update-docs bot commented Dec 21, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

Copy link
Collaborator

@JammingBen JammingBen left a comment

Choose a reason for hiding this comment

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

LGTM and works as expected 👍 Could you please add a changelog item for the bugfix?

Edit: e2e tests are failing though. You can run them locally via:

  • pnpm test:e2e:cucumber tests/e2e/cucumber/features/smoke/fullTextSearch.feature:6
  • pnpm test:e2e:cucumber tests/e2e/cucumber/features/smoke/search.feature:6

Maybe there is still a bug, or the tests need to be adjusted to the new behaviour.

@@ -129,6 +129,7 @@ export default defineComponent({
const optionsDropRef = ref(null)
const activePreviewIndex = ref(null)
const term = ref('')
const loadFromRoute = ref(false)
Copy link
Member

Choose a reason for hiding this comment

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

Small nitpick about the variable naming: I'd like to name this something like restoreSearchFromRoute to make it more descriptive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Collaborator

@JammingBen JammingBen left a comment

Choose a reason for hiding this comment

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

LGTM and works as expected AFAICT 👍 Could you please add a changelog item?

Copy link

sonarcloud bot commented Jan 5, 2024

Quality Gate Failed Quality Gate failed

Failed conditions

0.0% Condition Coverage on New Code (required ≥ 50%)

See analysis details on SonarCloud

Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

👍

@kulmann kulmann enabled auto-merge (squash) January 5, 2024 15:23
@kulmann kulmann merged commit 113e07f into owncloud:stable-8.0 Jan 8, 2024
2 of 3 checks passed
ownclouders pushed a commit that referenced this pull request Jan 8, 2024
* skipping searchbar preview on reload
ownclouders pushed a commit that referenced this pull request Jan 9, 2024
* skipping searchbar preview on reload
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.

None yet

3 participants