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: Change scroll container for sharing details #41444

Merged
merged 2 commits into from Nov 13, 2023
Merged

Conversation

juliushaertl
Copy link
Member

@juliushaertl juliushaertl commented Nov 13, 2023

Fix #41421
Fix #41304
Replaces #41424

Best to be reviewed without whitespace changes https://github.com/nextcloud/server/pull/41444/files?w=1 as I introduce one wrapping container for the new scroll area

Copy link
Contributor

@Fenn-CS Fenn-CS left a comment

Choose a reason for hiding this comment

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

It's better for the white space changes to first have commit followed by the actual change.

@juliushaertl
Copy link
Member Author

It's better for the white space changes to first have commit followed by the actual change.

Hm, I don't see a point in that, can you elaborate what the benefit would be? To me having one atomic change with all required adjustemnts seems more reasonable.

@ShGKme
Copy link
Contributor

ShGKme commented Nov 13, 2023

@juliushaertl Do you have understanding, what property and why fixes the problem with the scrolling?
Why there is a large padding in Julia's PR.

@Fenn-CS
Copy link
Contributor

Fenn-CS commented Nov 13, 2023

Hm, I don't see a point in that, can you elaborate what the benefit would be? To me having one atomic change with all required adjustemnts seems more reasonable.

It's confusing for me now and it would definitely be more confusing in future if anyone looks at the commit. The fix says "Change scroll container for sharing details" then the diff shows bunch for white space changes.

Unless I am missing something, I think it would be okay if the formatting changes only affected the areas that need to be reviewed. Otherwise, what if aside actual functional tests something is introduced in the unconcerned sections.

That said, given, there no strict commit culture here at Nextcloud, it would be helpful then to highlight the actual changes via comment or something or best in my opinion, separate the commits (for this and similar kind of changes in the future).

Copy link
Contributor

@Fenn-CS Fenn-CS left a comment

Choose a reason for hiding this comment

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

Tested works....

@juliushaertl
Copy link
Member Author

It's confusing for me now and it would definitely be more confusing in future if anyone looks at the commit. The fix says "Change scroll container for sharing details" then the diff shows bunch for white space changes.

Unless I am missing something, I think it would be okay if the formatting changes only affected the areas that need to be reviewed. Otherwise, what if aside actual functional tests something is introduced in the unconcerned sections.

That said, given, there no strict commit culture here at Nextcloud, it would be helpful then to highlight the actual changes via comment or something or best in my opinion, separate the commits (for this and similar kind of changes in the future).

Right, thanks for elaborating. This should be mostly relevant when running git blame where the -w -M flags are quite useful generally. I'd for now just extend the commit message to take the large whitespace change into account then.

@juliushaertl juliushaertl force-pushed the bugfix/41421 branch 2 times, most recently from 81fdd73 to 9ffbaf6 Compare November 13, 2023 20:47
In order to meet accessibility requirements we cannot use a sticky
footer that overlaps important interactable content. Therefore this
moves to a scroll container for the details which does not include the
action buttons at the bottom.

Larger indentation change in SharingDetailsTab was required to have a
wrapping container for the scroll area.

Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliushaertl
Copy link
Member Author

/compile

Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@juliushaertl
Copy link
Member Author

Do you have understanding, what property and why fixes the problem with the scrolling?
Why there is a large padding in Julia's PR.

I think the main lacking part was setting position: relative; on all wrapping containers as otherwise the flexbox would exceed the height even with enforcing 100%. The extra bottom margin was also a bit of a mystery to me tough.

@juliushaertl juliushaertl merged commit 32a485c into master Nov 13, 2023
38 of 41 checks passed
@juliushaertl juliushaertl deleted the bugfix/41421 branch November 13, 2023 23:13
@blizzz blizzz mentioned this pull request Nov 14, 2023
@AndyScherzinger AndyScherzinger added this to the Nextcloud 28 milestone Nov 14, 2023
@jancborchardt
Copy link
Member

Just one detail @juliushaertl – the "Delete share" button is not supposed to be sticky, but the last button in the content area. As proposed by @JuliaKirschenheuter and approved by Michael in #41304 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
6 participants