-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Set fileInfo correctly for LegacyTabs #20564
Conversation
Watch for changes of the fileInfo prop and propagate it to, the underlying component. Fixes nextcloud#20106. Signed-off-by: Christoph Seitz <christoph.seitz@posteo.de>
activeTab(activeTab) { | ||
if (activeTab === this.id && this.fileInfo) { | ||
this.setFileInfo(this.fileInfo) | ||
fileInfo(fileInfo) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It means every time the fileinfo changes, all the tabs gets reloaded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'am not sure, what you mean. The fileInfo is set once through the open method of the sidebar. (triggered through the Details click in the action list). Each tab get the fileInfo as prop from the sidebar. My change is just here to propagate the change to the component of the legacytab. Currently the fileInfo in the legacy component is only updates on the first load of the sidebar or when you change the tab back and forth (see the if clause). To fix the bug mentioned it needs to propagate every time the prop changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to prevent loading data in the background, I could leave both. The activeTab watch for changing tabs and the fileInfo if it is the activeTab. But I think there is no drawback of loading every thing at once as it makes switching tabs more seamless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I guess it's fine :)
I was concerned about the legacy tabs, but the vue ones should be smart enough to only refresh what is needed, so it's fine!
We'll kill the old tabs eventually! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good 👍
Yes, please. The bug is also present in 18 at least. |
/backport to stable18 |
backport to stable18 in #20588 with conflicts |
Watch for changes of the fileInfo prop and propagate it to the underlying component.
Fixes #20106.