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

UI problem with Grid Menu - is there a way to always have scroll on overflow-y ? #231

Closed
ghiscoding opened this issue Apr 6, 2018 · 2 comments

Comments

@ghiscoding
Copy link
Collaborator

ghiscoding commented Apr 6, 2018

I'm facing some visual problems with the Grid Menu that I worked on. It works as expected when the dataset is big and there are scrolling. However when the dataset is small and no scrolling appears, the Grid Menu shows up over the headerRow (see print screen 1)

I'd like to always have the overflow-y: scroll but when I do that, the grid isn't resized properly and another scrolling appears for the X axis. It somehow fixes the missing text of last column J and the filter header (see print screen 2).

I tried searching in the code, but I don't have good luck in finding how to resize the viewport minus the scroll width, which is the same width as the Grid Menu. That might be great if we could have an option to always show scroll on the Y axis, that way the Grid Menu would always work (that is what the UI Grid lib does, see their demo)

Column J header title and filter and not fully shown because of the Grid Menu overlapping, since Grid Menu requires scrolling (it takes same width as the scrolling bar)
gridmenu-noscroll

with scrolling always showing, it fixes Column J title and filter but I have to scroll left/right to see the entire grid.
gridmenu-withscroll

So the last print screen shows that it's almost there, if only it could be without the X axis scrolling.

@ghiscoding
Copy link
Collaborator Author

ghiscoding commented Apr 7, 2018

Oh I just found how to do it, if I set the variable viewportHasVScroll in slick.grid.js to always be True and my sample, I also set the grid option forceFitColumns: true OR I call autosizeColumns();, it works (see print screen).

@6pac
Would it possible to have a grid option to always set the viewportHasVScroll = true? Maybe an option name, alwaysShowVerticalScroll: true. With a simple option, it wouldn't affect anyone and it would help me fix the Grid Menu.

function updateRowCount() {
      ...
      var oldViewportHasVScroll = viewportHasVScroll;
      // with autoHeight, we do not need to accommodate the vertical scroll bar
-      // viewportHasVScroll = !options.autoHeight && (numberOfRows * options.rowHeight > viewportH);
+      viewportHasVScroll = true;
        viewportHasHScroll = (canvasWidth > viewportW - scrollbarDimensions.width);
     ...
}

and now it works :)
gridmenu-withfix

EDIT
I actually also need to set the overflow-y of slick-viewport to scroll for it to work. I currently have a patch, but if we can also put that in the code, that would be awesome.

.slick-viewport {
    overflow-y: scroll !important;
}

EDIT 2
If do the following modification in slick.grid.js, then I don't need the overflow-y patch in CSS.

var defaults = {
+      alwaysShowVerticalScroll: false,
        // ...
};

function init() {
   // ...
   $viewport = $("<div class='slick-viewport' style='width:100%;overflow:auto;outline:0;position:relative;;'>").appendTo($container);
-   $viewport.css("overflow-y", options.autoHeight ? "hidden" : "auto");
+   $viewport.css("overflow-y", options.alwaysShowVerticalScroll ? "scroll" : (options.autoHeight ? "hidden" : "auto"));
    $viewport.css("overflow-x", options.forceFitColumns ? "hidden" : "auto");
   // ...
}


function updateRowCount() {
   // ...
-  viewportHasVScroll = !options.autoHeight && (numberOfRows * options.rowHeight > viewportH);
+  viewportHasVScroll = options.alwaysShowVerticalScroll || !options.autoHeight && (numberOfRows * options.rowHeight > viewportH);
   // ...
}

@6pac
I can do a PR if you are ok with this new change. Let me know if you like and/or if there's anything to change/discuss. Thanks

@6pac
Copy link
Owner

6pac commented Apr 7, 2018

This is fine, Bring on a PR.

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 a pull request may close this issue.

2 participants