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

Don't scroll datepicker in AppSidebar with body #1881

Closed
wants to merge 1 commit into from

Conversation

raimund-schluessler
Copy link
Contributor

@raimund-schluessler raimund-schluessler commented Apr 26, 2021

This is the IMHO better but also more interfering solution to #998 than #1880 as it needs changes to AppSidebar and the datepicker library. In difference to #1880, the datepicker dropdown will not scroll with body.

In order to allow an absolutely positioned element (the datepicker) to overflow its ancestor although it has overflow: hidden set, the first positioned ancestor of the datepicker has to be an ancestor of the element with overflow: hidden as well (see https://css-tricks.com/popping-hidden-overflow/ for a bit of background and explanation).

This means we have to adjust AppSidebar to look like this:

<aside id="app-sidebar-vue" class="app-sidebar"> <!-- first positioned parent, position: sticky; -->
	<div class="app-sidebar-wrapper"> <!-- element with overflow-x: hidden; overflow-y: auto; -->
		<datepicker> <!-- absolutely positioned datepicker dropdown-->
	</div>
</aside>

Then the datepicker is positioned relative to aside and is allowed to overflow app-sidebar-wrapper. Since the datepicker library currently assumes that the datepicker is position: relative; it needs a fix to account for another element to be the first positioned ancestor. I have done this in https://github.com/raimund-schluessler/vue2-datepicker/commit/df040dbd5929f8f83f19390f396c924099ac7a38 and would send a PR to the datepicker library when we decide that this is a viable solution.

The great thing is that the datepicker is not cut-off, all scrolling inside the AppSidebar still works and the datepicker is static when scrolling body (tested in Firefox and Chrome):

datepicker_better

The changes to AppSidebar look more drastic than they actually are. I just added another div wrapping the AppSidebar, but this changes the indentation of all elements.
However, since the AppSidebar structure would change, custom adjustments maybe done by some apps might break (but should be fixable easily).

@raimund-schluessler raimund-schluessler added 3. to review Waiting for reviews breaking PR that requires a new major version bug Something isn't working feature: app-sidebar Related to the app-sidebar component feature: datepicker Related to the date/time picker component question Further information is requested labels Apr 26, 2021
@raimund-schluessler raimund-schluessler marked this pull request as ready for review April 26, 2021 09:21
@raimund-schluessler raimund-schluessler added this to the 4.0.0 milestone Apr 26, 2021
@nickvergessen
Copy link
Contributor

Better reviewed as https://github.com/nextcloud/nextcloud-vue/pull/1881/files?w=1

But this is not my level of expertise

Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
@raimund-schluessler
Copy link
Contributor Author

@nextcloud/vuejs @skjnldsv Ping 🙂
Could I have a decision if we want to pursue this further? If so, I would rebase the PR and push the necessary upstream changes further.

@raimund-schluessler raimund-schluessler modified the milestones: 5.0.0, 6.0.0 Feb 11, 2022
@@ -153,6 +154,11 @@ export default {
return new Date()
},
},

positionedParent: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add documentation

@skjnldsv
Copy link
Contributor

Could I have a decision if we want to pursue this further?

Honestly, the amount of changes to this PR is humongous :(
I really would not touch the AppSidebar component as this is stable. I am afraid of yet another brekage

@raimund-schluessler
Copy link
Contributor Author

Could I have a decision if we want to pursue this further?

Honestly, the amount of changes to this PR is humongous :( I really would not touch the AppSidebar component as this is stable. I am afraid of yet another brekage

Well, humongous is a bit exaggerated. Neglacting whitespace changes, the PR "only" adds another div to the sidebar as a wrapper (and adjusts the CSS to make this work). But I get your point, breaking something is not excluded for sure.
I guess we can live with the Datepicker scrolling with body then.

@raimund-schluessler raimund-schluessler deleted the fix/998/datepicker-cropped branch March 17, 2022 11:00
@skjnldsv
Copy link
Contributor

skjnldsv commented Mar 17, 2022

Well, humongous is a bit exaggerated

Sorry, that's a bit exaggerated yes 🙈
Also, I'm not the sole manager of this repo, maybe others are interested and/or can help review this PR.
But I've become too cautious nowadays 🙊

@raimund-schluessler raimund-schluessler removed this from the 6.0.0 milestone Mar 17, 2022
@raimund-schluessler raimund-schluessler removed the 3. to review Waiting for reviews label Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking PR that requires a new major version bug Something isn't working feature: app-sidebar Related to the app-sidebar component feature: datepicker Related to the date/time picker component question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants