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

fix: closing editors on search result page #9918

Merged
merged 1 commit into from
Nov 8, 2023

Conversation

JammingBen
Copy link
Contributor

Description

We noticed this error in the ocis pipeline: https://drone.owncloud.com/owncloud/ocis/28475/40/12. For some reason it didn't fail in Web...

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

@JammingBen JammingBen self-assigned this Nov 7, 2023
Copy link

update-docs bot commented Nov 7, 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.

@@ -186,6 +187,7 @@ export default defineComponent({
driveAliasAndItem
},
query: {
...unref(currentRoute).query,
fileId: id,
shareId: space.shareId,
...(unref(currentRoute).query?.app && { app: unref(currentRoute).query?.app }),
Copy link
Member

Choose a reason for hiding this comment

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

This here and above becomes unnecessary with your change, I'm wondering if this is really what we want or if this might cause issues somewhere, e.g. with the context route query params...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

I'm wondering if this is really what we want or if this might cause issues somewhere

You mean when re-setting all of the current route's query params? I decided against this initially. But apparently now other things break because params will get lost :/

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, maybe we can filter out some... iirc we had issues in some bizzare place when we had implemented it like this inititially, maybe login/logout...

Copy link
Member

Choose a reason for hiding this comment

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

Ah, misreading the diff.. issue happened in closeApp iirc. So if this is feally only overriding a few params but stays on the same page l otherwise, it might be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems so, at least I didn't find any other issues for now.

@JammingBen JammingBen force-pushed the fix-closing-editors-search-result-page branch from da9ae2a to 8700cde Compare November 8, 2023 07:23
Copy link

sonarcloud bot commented Nov 8, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell C 5 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@JammingBen JammingBen requested a review from dschmidt November 8, 2023 08:19
@JammingBen JammingBen merged commit 9a95076 into master Nov 8, 2023
3 of 4 checks passed
@delete-merged-branch delete-merged-branch bot deleted the fix-closing-editors-search-result-page branch November 8, 2023 08:34
ownclouders pushed a commit that referenced this pull request Nov 8, 2023
…ult-page

fix: closing editors on search result page
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.

2 participants