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

Show a floating horizontal scrollbar for tables #1092

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cproensa
Copy link
Contributor

@cproensa cproensa commented Apr 14, 2017

Show a floating horizontal scrollbar for tables whose horizontal
overflow is hidden under the container, and the native horizontal
scrollbar is out of view when the length of the table exceeds the
browser view area.

Using JS code:
https://github.com/Amphiluke/floating-scroll

Fixes: #22132

Before:
seleccion_167

After:
seleccion_166

Copy link
Contributor

@syncguru syncguru left a comment

Choose a reason for hiding this comment

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

Not sure why floating-scroll is used here? The plugin seems to have very limited use thus far so taking a dependency on it is not acceptable practice for mantisbt core.

@@ -271,6 +271,9 @@ function layout_head_css() {
html_css_link( 'bootstrap-datetimepicker-' . DATETIME_PICKER_VERSION . '.min.css' );
}

# floating scroll (no cdn available)
html_css_link( 'jquery.floatingscroll-' . FLOATING_SCROLL_VERSION . '.css' );
Copy link
Contributor

@syncguru syncguru Apr 15, 2017

Choose a reason for hiding this comment

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

Why using another library for this? Is this not possible with perfect-scrollbar plugin that I have in use @ #1088?

@cproensa
Copy link
Contributor Author

Not sure why floating-scroll is used here? The plugin seems to have very limited use thus far so taking a dependency on it is not acceptable practice for mantisbt core.

It's a one-thing focused piece of code, that solves exactly the issue
I'm open to any alternatives, but i composed this as nobody have cared to do so far, and for me this is preventing upgrading to v2.

Why using another library for this? Is this not possible with perfect-scrollbar plugin that I have in use @ #1088?

Did you review if the use of perfect-scrollbar in that PR can be equivalent to just "overflow-y:auto;" the timeline container? #1088 (comment)

Additionally, i tried but couldn't make perfect-scrollbar work for this scenario. Let me know if you find how to do it, if have more experience with that library.

@syncguru
Copy link
Contributor

Additionally, i tried but couldn't make perfect-scrollbar work for this scenario. Let me know if you find how to do it, if have more experience with that library.

Wait... you want a scrollbar (horizontal & vertical) to be visible at all times if the table is larger than the screen, right? Did the legacy UI had this? Seems to go counter to how modern browsers handle scrollbar (desktop & mobile). They are always hidden.

@cproensa
Copy link
Contributor Author

Wait... you want a scrollbar (horizontal & vertical) to be visible at all times if the table is larger than the screen, right?

Not "if the table is larger than the screen", that is the standard browser scrollbar, where an element is out of screen.
I mean, when the table is larger than the container.

Look carefully, you'll see that the table container is never wider than the screen, but the content (table) is wider than the container. In this situation, the browser draws a standard horizontal scrollbar at the bottom of the container

The table container spreads vertically, so the vertical scrollbar applies to the body, and is showed as its standard behaviour.
But, the horizontal scrollbar is not visible because the bottom of the container is off the screen. This is the issue i'm trying to address.

Please try to reproduce this with a combination of enough content width and/or browser width, to understand the issue.

Did the legacy UI had this?

Old UI did not have this issue, because tables were not constrained to its container size, so the browser showed a standard horizontal scrollbar on the body always visible, whenever the table+container was wider than screen.

Seems to go counter to how modern browsers handle scrollbar (desktop & mobile). They are always hidden.

That's not applicable to this scenario.

@syncguru
Copy link
Contributor

I think the snapshots are a bit confusing - couple of things:
1- why vertical scroll bar is visible?
2- does the horizontal scroll bar disappear if no left/right scrolling happening? Just like the vertical scroll bars do?

@syncguru
Copy link
Contributor

The behavior in the demo page at http://amphiluke.github.io/floating-scroll/ is exactly what I am looking for. The horizontal scrollbar appears when I try to scroll and disappears shortly after scroll movement is done.

@cproensa
Copy link
Contributor Author

why vertical scroll bar is visible?

because the table container overflows past the browser screen. That is the standard layout, and no changes are intended on that, eg: limiting vertical height for the table container

The behavior in the demo page at http://amphiluke.github.io/floating-scroll/ is exactly what I am looking for.

The behaviour here is the same as the demo page. Note that the included script is very concise and is focused only in this feature, nothing else.

does the horizontal scroll bar disappear if no left/right scrolling happening?

Not exactly.
The horizontal bar gets sticky at the bottom visible limit of the container when the conditions happen: there is already a native horizontal bar, and, said native bar is not visible out of screen
There's no added bar when the table fits in the container, so from a UI persepective, it's safe to apply to all table-responsive containers (any data table is potentially affected)

What you are experiencing, where the scroll bars get hidden after the movement is done, is surely dependent on your browser (I don't see that in any scroll bar with my browser).
However,said behavior for the scripted bar is exactly the same as the one native bars have.

You can pull the PR tree and test it to make sure.

Copy link
Contributor

@syncguru syncguru left a comment

Choose a reason for hiding this comment

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

Other than the two comments I have, this is good to go.

@@ -0,0 +1,34 @@
.fl-scrolls, .fl-scrolls div {
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't check-in non-minified css/js for public plugins.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the plugin does not offer a minified css.
Probably, because it contains only 8 rules...

@@ -0,0 +1,166 @@
/*!
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

@cproensa
Copy link
Contributor Author

Other than the two comments I have

Already, removed non minified js.
But the plugin does not provide minifed css. Is it preferable to leave this as is?

@cproensa
Copy link
Contributor Author

any feedback on this?
This is still an unresolved problem in released versions

@vboctor
Copy link
Member

vboctor commented Dec 31, 2017

@cproensa I can't seem to notice any difference between master and behavior in this PR. Is this still working?

@cproensa
Copy link
Contributor Author

This is old but still valid
I introduced it manually in my instances and has worked fine ever since.

@dregad
Copy link
Member

dregad commented Mar 4, 2019

@cproensa merge of #1449 introduced conflicts in this one. They are quite straightforward and I could easily fix them, but I noticed that the floating-scroll library has been updated since then (2.3.3 and 3.0.4, the latter drops support for IE8 but our minimum req is IE11 so we should be fine with that I guess), so I let you decide if you want to update or not.

@cproensa
Copy link
Contributor Author

cproensa commented Mar 4, 2019

Thanks for the update. It's a good sign that this project is active.
I will modify this branch, and have a test run with the latest version.

Show a floating horizontal scrollbar for tables whose horizontal
overflow is hidden under the container, and the native horizontal
scrollbar is out of view when the length of the table exceeds the
browser view area.

Using JS code:
https://github.com/Amphiluke/floating-scroll

Fixes: #22132
@cproensa
Copy link
Contributor Author

cproensa commented Mar 4, 2019

I have rebased and updated this branch with the new version, i will run it in production for the following days, but so far it doesn't show issues.

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