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

fix size of modal-container #2030

Closed
wants to merge 1 commit into from
Closed

fix size of modal-container #2030

wants to merge 1 commit into from

Conversation

szaimen
Copy link
Contributor

@szaimen szaimen commented Oct 26, 2023 •
edited
Loading

@szaimen
fix size of modal-container
0660872 
Signed-off-by: Simon L <szaimen@e.mail.de>
@szaimen szaimen added bug Something isn't working 2. developing Work in progress labels Oct 26, 2023
@szaimen szaimen added this to the Nextcloud 28 milestone Oct 26, 2023
@szaimen szaimen marked this pull request as ready for review October 26, 2023 12:19
@szaimen szaimen added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 26, 2023
@szaimen szaimen mentioned this pull request Oct 26, 2023
Merged
danxuliu
danxuliu requested changes Oct 28, 2023
View reviewed changes
Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

The space at the bottom is explicitly added, and it is used by default for all handlers (even if it may be overriden later), so I do not think that clearing it to fix the PDF viewer is the right approach (but please note that I have not actually checked how this change affects other handlers).

Moreover, that bottom space has been there since Nextcloud 25 and it only started to be a problem in the PDF viewer in Nextcloud 28.

Please refer to nextcloud/files_pdfviewer#843 (comment) for an explanation of the issue. Unfortunately I am not sure of what would be a good solution 🤔

@szaimen
Copy link
Contributor Author

szaimen commented Oct 30, 2023

The space at the bottom is explicitly added, and it is used by default for all handlers (even if it may be overriden later), so I do not think that clearing it to fix the PDF viewer is the right approach (but please note that I have not actually checked how this change affects other handlers).

Moreover, that bottom space has been there since Nextcloud 25 and it only started to be a problem in the PDF viewer in Nextcloud 28.

Please refer to nextcloud/files_pdfviewer#843 (comment) for an explanation of the issue. Unfortunately I am not sure of what would be a good solution 🤔

I chose this solution because for me it doesnt make sense from design perspective to enforce the space on the bottom. WDYT @nextcloud/designers?

@skjnldsv
Copy link
Member

skjnldsv commented Oct 31, 2023 •
edited
Loading

I chose this solution because for me it doesnt make sense from design perspective

Visual balance! It looks weird if the top spacing isn't matching the bottom one. This was seen and requested by the design team in the past.
Maybe let's not ping the whole design team for a previously taken decision? 🤷

With Without
dev skjnldsv com_apps_files_files_419354_dir=_Photos (1) dev skjnldsv com_apps_files_files_419354_dir=_Photos

@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
Merged
@szaimen
Copy link
Contributor Author

szaimen commented Nov 1, 2023

Visual balance! It looks weird if the top spacing isn't matching the bottom one. This was seen and requested by the design team in the past. Maybe let's not ping the whole design team for a previously taken decision? 🤷

With Without
dev skjnldsv com_apps_files_files_419354_dir=_Photos (1) dev skjnldsv com_apps_files_files_419354_dir=_Photos

You are right. I guess we need to find a different solution then in pdf viewer...

@szaimen szaimen closed this Nov 1, 2023
@szaimen szaimen deleted the enh/noid/fix-modal-size branch November 1, 2023 10:12
@szaimen
Copy link
Contributor Author

szaimen commented Nov 1, 2023

Actually found an easy fix in the pdfviewer: nextcloud/files_pdfviewer#845

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danxuliu danxuliu danxuliu requested changes

@artonge artonge Awaiting requested review from artonge

@skjnldsv skjnldsv Awaiting requested review from skjnldsv

Assignees
No one assigned
Labels
3. to review Waiting for reviews bug Something isn't working
Projects
None yet
Milestone
Nextcloud 28
Development

Successfully merging this pull request may close these issues.

PDF-viewer not full-screen anymore on NC28
3 participants
@szaimen @skjnldsv @danxuliu