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 #1668 #1693

Closed
wants to merge 1 commit into from
Closed

fix #1668 #1693

wants to merge 1 commit into from

Conversation

nkappler
Copy link
Contributor

@nkappler nkappler commented Jan 27, 2024

this PR enables scrolling for the menu popovers by restricting their height (or the entire menu height, respectively) to the viewport, both in expanded and collapsed (hamburger) state:
grafik
grafik

One question related to the contributing process:
What are the benefits of manually merging each PR?
It seems to me that it's just additional effort, plus keeping forks up to date cannot be done automatically by github, since you end up with duplicate commits.

@tbnobody
Copy link
Owner

tbnobody commented Jan 27, 2024

There are no duplicate forks as the git hash stays the same. I just merge it manually because I doublecheck each PR.
Just for some PRs I squash and reword stuff. (and yes in this case it's really a different commit)
And as long as you don't add this PR to multiple projects, everything is fine

@tbnobody
Copy link
Owner

With this PR I see scrollbars on every menu even if the screen is large enough

image

@nkappler
Copy link
Contributor Author

Good catch, at some point I even wondered why I didn't see those, I guess I've spent all my time in the mobile emulator view 😅
I'm going to fix it probably later today...

@nkappler
Copy link
Contributor Author

There are no duplicate forks as the git hash stays the same. I just merge it manually because I doublecheck each PR. Just for some PRs I squash and reword stuff. (and yes in this case it's really a different commit) And as long as you don't add this PR to multiple projects, everything is fine

Yes its also easily avoidable by strictly developing on feature branches, I only was curious 😃

@nkappler
Copy link
Contributor Author

nkappler commented Jan 27, 2024

I'll need some more time for this, it isn't trivial to hide the scrollbar when it is not needed without introducing more dom nodes.
I'm going to need to read some documentation too.

I've just stumbled upon a nice new css property draft: scrollbar-width
Unfortunately browser support is just now getting started so I don't consider it a valid option (yet):

I also don't like to use media queries, although we could only enable this css on small screens or mobile devices in general.

@tbnobody what do you think? any suggestions or feedback at this point?

@nkappler nkappler force-pushed the fix_1668 branch 4 times, most recently from a2fff84 to b2facd9 Compare January 28, 2024 00:11
@nkappler
Copy link
Contributor Author

nkappler commented Jan 28, 2024

I've got a working solution now for the hamburger menu, which consisted of joining the two menu lists into one, using an empty list element which can flex-grow into the available space, so visually nothing changes.
I then applied the builtin bootstrap css class for scrolling navs which seems to work just as you would expect.

This means that the dropdown poppers won't scroll when they don't fit on the screen, but I don't think its a huge issue since this would only appear on very strange screen resolutions.
I have tried the boundary attribute, which sounded like it should work, but it didn't seem to have any effect, but maybe I'm just doing it wrong, after all I am still very new to vue.js and bootstrap.

@nkappler
Copy link
Contributor Author

I've just read through the popper.js documentation again, it seems like the boundary property is only for positioning purposes, not sizing purposes, but as I said, the main problem from the bug report is now fixed properly with this PR.

Let me know if you want the other issue fixed too, then I can look into it again

@tbnobody
Copy link
Owner

Looks good. But could you please remove the changes in the lines where you just changed the line breaks and squash the commits into one?

@nkappler
Copy link
Contributor Author

nkappler commented Feb 2, 2024

Hi, sorry for this, I have an autoformatter installed for work...
I have reverted the line breaks now.
Regarding the Squash, you should be able to just merge the PR as a squash merge, can you?
grafik

@nkappler
Copy link
Contributor Author

nkappler commented Feb 8, 2024

here you go, one commit 😃

@tbnobody
Copy link
Owner

tbnobody commented Feb 8, 2024

Sorry that I didn't mentioned it... I've already included it in my local dev branch. will be included in the next release.

@tbnobody
Copy link
Owner

Merged in 86b9625

@tbnobody tbnobody closed this Feb 12, 2024
Copy link

github-actions bot commented Apr 1, 2024

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new discussion or issue for related concerns.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants