Skip to content

Conversation

@Antreesy
Copy link
Contributor

@Antreesy Antreesy commented Aug 14, 2025

☑️ Resolves

  • ref [vue3] Fix Files Sidebar #12382
  • drop legacy registerSecondaryView
  • remove redundant extra app and some logic around it
  • refactor callview component in sidebar

🖌️ UI Checklist

🖼️ Screenshots / Screencasts

🏚️ Before 🏡 After
image image

🚧 Tasks

  • Close button breaks call view if accidentally clicked
    • temporary disabled
    • there is no case you would need to click it before leaving the call
    • Should be completely hidden, as in Public sidebar?
  • Bundled size is increased for integration, Should look into optimizing initial load (see previous PR)

🏁 Checklist

  • 🌏 Tested with different browsers / clients:
    • Chromium (Chrome / Edge / Opera / Brave)
    • Firefox
    • Safari
    • Talk Desktop
    • Integrations with Files sidebar and other apps
    • Not risky to browser differences / client
  • 🖌️ Design was reviewed, approved or inspired by the design team
  • ⛑️ Tests are included or not possible

@Antreesy Antreesy added this to the 🪺 Next Major (32) milestone Aug 14, 2025
@Antreesy Antreesy requested review from DorraJaouad and ShGKme August 14, 2025 16:13
@Antreesy Antreesy self-assigned this Aug 14, 2025
@Antreesy Antreesy added 3. to review technical debt feature: frontend 🖌️ "Web UI" client feature: talk-sidebar ⬅️ Sidebar integration of Talk into other apps like sharing and documents labels Aug 14, 2025
@Antreesy Antreesy force-pushed the fix/noid/files-sidebar branch from 1be164a to aea7657 Compare August 14, 2025 16:13
@Antreesy Antreesy force-pushed the fix/noid/files-sidebar branch from aea7657 to 38f1f29 Compare September 1, 2025 16:29
@ShGKme
Copy link
Contributor

ShGKme commented Sep 12, 2025

  • drop legacy registerSecondaryView

Is it legacy/deprecated API?

@Antreesy
Copy link
Contributor Author

Is it legacy/deprecated API?

according to nextcloud/server#54337 it's a part of legacy, and with Vue3 Teleport I pretty much do not see a need for using it

@ShGKme
Copy link
Contributor

ShGKme commented Sep 13, 2025

according to nextcloud/server#54337 it's a part of legacy

As I understand it, only the FileInfoModel is the problematic part.

@ShGKme
Copy link
Contributor

ShGKme commented Sep 13, 2025

with Vue3 Teleport I pretty much do not see a need for using it

It works, but IMO it is a very indirect API. We use the internal layout of the sidebar (class names and HTML layout). Sidebar devs don't know we are teleporting something to a specific part of the sidebar

@ShGKme
Copy link
Contributor

ShGKme commented Sep 13, 2025

All 5 commits are fix(FilesSidebar), but I don't understand, what is being fixed, except for the disabled button during a call.

Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

Since we have much code relying on sidebar internals like

document.querySelector('header.app-sidebar-header')

using <teleport> does not make anything more risky than it is at the moment.

Other refactorings remove some of the style overrides, which is also good.

Please, follow conventional commits.

  • "a commit of the type fix patches a bug in your codebase"
    • So a commit with fix should resolve an actual problem
  • We can use refactor (or chore) for refactoring
  • wip: can be used for intermediate commits when the application is not in a working state, and checkout out commit breaks the app/build

it makes review easier by specifying the purpose of the change.

…e app

- drop usage of `registerSecondaryView` - can be implemented with lesser DOM modifications (and same risk of breaking)

Signed-off-by: Maksim Sukharev <[email protected]>
- this allows to drop all redundant handling of file info

Signed-off-by: Maksim Sukharev <[email protected]>
@Antreesy Antreesy force-pushed the fix/noid/files-sidebar branch from 38f1f29 to 4e0e1ae Compare September 15, 2025 17:08
@Antreesy Antreesy force-pushed the fix/noid/files-sidebar branch from 4e0e1ae to ac6f5b3 Compare September 15, 2025 17:09
@Antreesy
Copy link
Contributor Author

/backport to stable32

@Antreesy Antreesy merged commit f9727b6 into main Sep 15, 2025
55 checks passed
@Antreesy Antreesy deleted the fix/noid/files-sidebar branch September 15, 2025 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review feature: frontend 🖌️ "Web UI" client feature: talk-sidebar ⬅️ Sidebar integration of Talk into other apps like sharing and documents technical debt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants