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.

Sign up for GitHub

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

Jump to bottom

Disable rich workspace rendering when view is not visible #3349

Merged
merged 4 commits into from
Nov 7, 2022
Merged

Disable rich workspace rendering when view is not visible #3349

merged 4 commits into from
Nov 7, 2022

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Nov 2, 2022 •
edited
Loading

As the file list is just hidden by the files app the vue component would still be active when switching to recent files for example.

@juliusknorr juliusknorr requested review from vinicius73, azul, max-nextcloud, mejo- and luka-nextcloud and removed request for vinicius73 and azul November 2, 2022 12:45
@juliusknorr juliusknorr added bug Something isn't working 3. to review labels Nov 2, 2022
@juliusknorr
Copy link
Member Author

The event itself is available since 24 by the backports of nextcloud/server#34100

@juliusknorr juliusknorr changed the title bugfix/noid/workspace filelists Disable rich workspace rendering when view is not visible Nov 2, 2022
mejo-
mejo- reviewed Nov 2, 2022
View reviewed changes
Copy link
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

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

The fix works well when tested locally. Cypress tests seem to need some love though.

@juliusknorr
Copy link
Member Author

Strange, it was passing fine locally, but I'll dive into that.

@juliusknorr @max-nextcloud
Do not render the workspace if file list is hidden
5812336 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr @max-nextcloud
Add cypress tests for switching views with rich workspaces
f85a3cd 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@max-nextcloud max-nextcloud force-pushed the bugfix/noid/workspace-filelists branch from 8b3b3d1 to f85a3cd Compare November 7, 2022 14:00
max-nextcloud
max-nextcloud requested changes Nov 7, 2022
View reviewed changes
Copy link
Collaborator

@max-nextcloud max-nextcloud left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Code looks good.

Requesting some minor changes in tests that will make retries use consistent folder names.

They don't necessarily address the failures in CI though.

cypress/e2e/workspace.spec.js Outdated Show resolved Hide resolved
cypress/e2e/workspace.spec.js Outdated Show resolved Hide resolved
@cypress
Copy link

cypress bot commented Nov 7, 2022 •
edited
Loading


Test summary

103 • 1 • 0 • 0 • Flakiness 1

Run details
Project Text
Status Failed
Commit d5748e0 ℹ️
Started Nov 7, 2022 3:13 PM
Ended Nov 7, 2022 3:20 PM
Duration 07:01 💡
OS Linux Ubuntu - 20.04
Browser Electron 106

View run in Cypress Dashboard ➡️

Failures

cypress/e2e/links.spec.js 1 Failed
1 test link marks > link preview > shows a link preview

Flakiness

cypress/e2e/workspace.spec.js 1 Flakiness
1 Workspace > formats text

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@max-nextcloud
Fix minor issues in cypress tests leading to failures
9fed8ea 
* `#workspace` is not found on the recent pages view at all - so need to combine selectors
* allow tests to pass in retries by using a consistent path name.

Signed-off-by: max-nextcloud <max@nextcloud.com>
mejo-
mejo- approved these changes Nov 7, 2022
View reviewed changes
Copy link
Member

@mejo- mejo- 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 fixing the tests @max-nextcloud.

@mejo-
Copy link
Member

mejo- commented Nov 7, 2022

/compile

@nextcloud-command
Compile assets
b48c03f 
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@mejo-
Copy link
Member

mejo- commented Nov 7, 2022

/backport 5812336,f85a3cd5c996dbfe8d2230859d67ae5c17847242,9fed8ea72469d6d25c6723689aa5c2b634ea1a52 to stable25

@juliusknorr
Copy link
Member Author

Failures/flaky tests are known, merging

@juliusknorr juliusknorr merged commit 03cfafa into master Nov 7, 2022
@juliusknorr juliusknorr deleted the bugfix/noid/workspace-filelists branch November 7, 2022 16:14
@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Nov 7, 2022
Merged
1 task
@mejo-
Copy link
Member

mejo- commented Nov 7, 2022

backport 5812336,f85a3cd5c996dbfe8d2230859d67ae5c17847242,9fed8ea72469d6d25c6723689aa5c2b634ea1a52 to stable24

@mejo-
Copy link
Member

mejo- commented Nov 7, 2022

/backport 5812336,f85a3cd5c996dbfe8d2230859d67ae5c17847242,9fed8ea72469d6d25c6723689aa5c2b634ea1a52 to stable24

@backportbot-nextcloud
Copy link

The backport to stable24 failed. Please do this backport manually.

@juliusknorr juliusknorr mentioned this pull request Nov 9, 2022
Merged
@blizzz blizzz mentioned this pull request Feb 9, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mejo- mejo- mejo- approved these changes

@max-nextcloud max-nextcloud max-nextcloud approved these changes

@luka-nextcloud luka-nextcloud Awaiting requested review from luka-nextcloud

@vinicius73 vinicius73 Awaiting requested review from vinicius73

Assignees
No one assigned
Labels
3. to review bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Disable text app fetching if switching to other lists (Recents) Extend cypress tests
4 participants
@juliusknorr @mejo- @max-nextcloud @nextcloud-command