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 taking sidebar position in fullscreen mode #1711

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Jun 8, 2023

🐞 Resolves

Steps to reproduce:

  1. Open files
  2. Open a file sidebar
  3. On this moment Viewer handle sidebar state change and updates the size
  4. Open a file
  5. Sidebar goes full-screen mode
  6. Viewer is opened (with the old size)
  7. There is a gap

🖼️ Changes

Take the sidebar position in beforeOpen

Before

master.mp4

After

FIX.mp4
Old description

A bug

How Viewer react on sidebar opening now in Files:

  1. Viewer is open, sidebar is closed:
    State-1
  2. Click "Open Sidebar"
  3. Sidebar is opened in the default layout
    State-2
    • File info is loading
    • Viewer takes it's position to set its width
  4. Loading is finished. Sidebar goes fullscreen. But viewer still use old position for width
    State-3
  5. There is a gap between Viewer and Sidebar

Exactly the same happens when Viewer is opened having Sidebar already opened.

It wasn't a problem before #1640 because incorrect width early was correct for fullscreen mode.

Expected behavior

No gap

State-4

Solution in this PR

Retake Sidebar's position when file opening (including transition animation) is finished.

Drawbacks

Because of sub-loading and 100ms transition on width, for a short time there is a gap anyway and animation if it's disappearing...

gap.mp4

Alternative solutions

Alternative solutions could be:

  1. Always use Sidebar in the fullscreen mode with Viewer. Then we can just always use width.
  2. Change Sidebar, make it Fullscreen immediately, not after all data is loaded
  3. Add a ghost element directly in DOM inside Sidebar and use ResizeObserver
  4. Partially revert fix: use left position of sidebar to set viewer width #1640 and keep Files accurate having overlap in other apps without fullscreen sidebar.

@ShGKme ShGKme added bug Something isn't working 2. developing Work in progress labels Jun 8, 2023
@ShGKme ShGKme self-assigned this Jun 8, 2023
@ShGKme ShGKme force-pushed the fix/viewer-width-with-sidebar-in-fullscreen-mode branch from 5730cc6 to 80ca372 Compare June 8, 2023 21:52
@ShGKme
Copy link
Contributor Author

ShGKme commented Jun 8, 2023

Rebased onto master + compiled in prod mode

@ShGKme ShGKme force-pushed the fix/viewer-width-with-sidebar-in-fullscreen-mode branch from 80ca372 to c002a26 Compare June 8, 2023 21:54
@juliushaertl
Copy link
Member

Just noticed this existed after working on an alternative approach at nextcloud/server#39378, but it is basically alternative 1 you considered. Would appreciate input on that one as it seems to have the same effect without the mentioned drawback from above.

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme ShGKme force-pushed the fix/viewer-width-with-sidebar-in-fullscreen-mode branch from c002a26 to f4d96e1 Compare July 28, 2023 12:56
@ShGKme ShGKme requested a review from juliushaertl July 28, 2023 13:13
@ShGKme
Copy link
Contributor Author

ShGKme commented Jul 28, 2023

PR is updated after nextcloud/server#39378

@ShGKme ShGKme marked this pull request as ready for review July 28, 2023 13:13
@ShGKme
Copy link
Contributor Author

ShGKme commented Aug 8, 2023

@juliushaertl @skjnldsv @artonge
Sorry for mentions :)
I'll check Cypress tests if everything looks fine by code/solution.

src/views/Viewer.vue Show resolved Hide resolved
@blizzz blizzz added this to the Nextcloud 29 milestone Nov 23, 2023
@blizzz
Copy link
Member

blizzz commented Nov 23, 2023

Please don't forget to set the milestone when opening a PR 😃

@skjnldsv skjnldsv added this to the Nextcloud 30 milestone Mar 28, 2024
@skjnldsv skjnldsv mentioned this pull request Aug 22, 2024
44 tasks
@blizzz blizzz modified the milestones: Nextcloud 30, Nextcloud 31 Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants