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: Left toolbar scrollable #2041

Closed
wants to merge 2 commits into from
Closed

Conversation

jokd
Copy link
Contributor

@jokd jokd commented Sep 11, 2024

Fixes #2021
Makes left toolbar scrollable if too long for browser window

@steff-o
Copy link
Contributor

steff-o commented Sep 11, 2024

What is the rationale of using an id-selector instead of class? Wouldn't that cause problems if a page hosts several origo applications using different app-wrapper containers?

While it currently probably is not possible to actually host several origo-applications in the same document, I think it unnecessary to make it even harder to make it possible. Not that I have no idea why anyone would like to host several instances in the same document, but anyway. Overview map maybe?

@jokd
Copy link
Contributor Author

jokd commented Sep 12, 2024

What is the rationale of using an id-selector instead of class? Wouldn't that cause problems if a page hosts several origo applications using different app-wrapper containers?

While it currently probably is not possible to actually host several origo-applications in the same document, I think it unnecessary to make it even harder to make it possible. Not that I have no idea why anyone would like to host several instances in the same document, but anyway. Overview map maybe?

Due to specificity and a bit of laziness. The .box.transparent css rule sets overflow:visible which we don't want and to ensure that rule is overridden I used the id selector. Otherwise we need to change or create new classes for the toolbar element.

@steff-o
Copy link
Contributor

steff-o commented Sep 12, 2024

The id-selector issue aside (it would work with multiple identical ids, it just is "wrong" to have multiple identical id:s and it is just a hypothetical case anyway), I think it is pretty limited and hard to discover. There is no indication what so ever that it is possible to scroll the toolbar and it only works with scroll wheel, which makes it useless on phones which probably is the situation where it would be most useful.

Just for fun I tried to show the toolbar but it appeared on the far right of the screen and was not scrollable by dragging the bar, possibly due do OL:s interactions taking precedence.

Also when the screen gets small the attributions control changes from a bar at the bottom to an icon at bottom left, which will overlay the toolbar. The scrollable panel would have to compensate for that area as well, even if it is possible to scroll the last tool above the "i" icon as there seems to be an empty row last in the toolbar.

image

Ideally there would appear clickable scroll up and scroll down arrows above and below the tool bar and the tool buttons themselves should be possible to drag in order to scroll the toolbar, but that will take implementation to a completely new level.

@jokd
Copy link
Contributor Author

jokd commented Sep 12, 2024

it only works with scroll wheel, which makes it useless on phones which probably is the situation where it would be most useful.

Now I recall why the tooltips wouldn't work the last time I looked into it. pointer-events can't be auto when the div is broader than the toolbar (which is necessary to show the tooltips) and keeping pointer-events: none will prevent touch scrolling. I close this now and maybe will get back to it later.

@jokd jokd closed this Sep 12, 2024
@jokd jokd deleted the left-tools-scroll branch September 12, 2024 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The toolbar needs to know the size of the browser
2 participants