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: Always apply fullscreen styling to sidebar #39378

Merged
merged 2 commits into from
Jul 18, 2023

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Jul 13, 2023

Summary

Make sure that the full screen styling is always applied to the sidebar even if the loading hasn't finished yet.

Fixes a small UI glitch where a small space was left between the viewer modal and the sidebar.

This is a different approach I came up before stumbling over nextcloud/viewer#1711 so I'd appreciate feedback especially from @ShGKme on this. It seems to also cover the drawback that the viewer PR has.

Before

Screenshot 2023-07-13 at 22 15 23

After

Screen.Recording.2023-07-13.at.22.14.29.mov

TODO

  • ...

Checklist

@juliusknorr juliusknorr requested review from skjnldsv, ShGKme, a team, susnux and szaimen and removed request for a team July 13, 2023 20:18
@juliusknorr juliusknorr added bug design Design, UI, UX, etc. 3. to review Waiting for reviews labels Jul 13, 2023
@juliusknorr juliusknorr self-assigned this Jul 13, 2023
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.

I like this solution!

I didn't finish that PR in viewer because didn't find a good solution from the viewer side but didn't want to modify the server part :D
But this is definitely better.


Unfortunately, it still doesn't work in one case:

  1. Open files
  2. Open sidebar
  3. Open viewer

image

It works in combination with my PR in viewer, but with a 0.2s transition...

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.

Approved, regardless viewer, IMO it is better to have correct fullscreen state in all sidebar states in files.

Also, I'd consider to make fullscreen mode NcAppSidebar component feature, so we would be able to use it in any apps and have better sidebar integration in apps (for example, having shared fullscreen / resize events).

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

@juliushaertl in your screen recording the viewer and sidebar open and close visibly at slightly different times when clicking the close "x" and the file name respectively. Is this something new based on the approach? (Maybe it’s also just because of the screen recording.)

@juliusknorr
Copy link
Member Author

juliusknorr commented Jul 17, 2023

This is already the case before this PR, I'm purely targeting the weird spacing issue here.

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Looks good design-wise.

Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr juliusknorr force-pushed the bugfix/noid/sidebar-fullscreen branch from f2400ff to ac13226 Compare July 17, 2023 19:26
@juliusknorr
Copy link
Member Author

/compile

Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug design Design, UI, UX, etc.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants