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

resizeCanvas() does not take into account the preHeader height #493

Closed
xavier-90 opened this issue May 16, 2020 · 6 comments
Closed

resizeCanvas() does not take into account the preHeader height #493

xavier-90 opened this issue May 16, 2020 · 6 comments

Comments

@xavier-90
Copy link

I have created a grid with the wollowing options:

                        autoEdit: false,
                        editorCellNavOnLRKeys: true,
                        enableAutoResize: true,
                        enableCellNavigation: true,
                        enableColumnReorder: false,
                        enableGrouping: false,
                        frozenColumn: 0,
                        createPreHeaderPanel: true,
                        editable: true,
                        explicitInitialization: true,
                        preHeaderPanelHeight: 25,
                        showPreHeaderPanel: true,
                        autoResize: {
                        containerId: $('#MyGrid')

On calling resizeCanavas(), e.g.

                         $(window).resize(function () {
                             grid.resizeCanvas();
                         });

the height of the grid is reduced by 25px.

@ghiscoding
Copy link
Collaborator

ghiscoding commented May 16, 2020

Can you try to modify the slick.grid.js file and the resizeCanvas on line 3449 on your side with the following changes and see if that is enough? I don't fully know how to test it, so I would rather you confirm the change works as you intend it.

function resizeCanvas() {
      if (!initialized) { return; }
      paneTopH = 0;
      paneBottomH = 0;
      viewportTopH = 0;
      viewportBottomH = 0;
+      var preHeaderHeight = options.showPreHeaderPanel ? options.preHeaderPanelHeight : 0;

  //-- ... code removed for brevity --//

      // The top pane includes the top panel and the header row
+      paneTopH += topPanelH + preHeaderHeight + headerRowH + footerRowH;

      if (hasFrozenColumns() && options.autoHeight) {
        paneTopH += scrollbarDimensions.height;
      }

      // The top viewport does not contain the top panel or header row
+      viewportTopH = paneTopH - topPanelH - preHeaderHeight - headerRowH - footerRowH;

EDIT

Actually I tried with the Auto-Resize Example by adding a blank pre-header, and I think it works with these changes. It would be nice if you can confirm on your side before merging PR #497 with the fix.

image

@xavier-90
Copy link
Author

xavier-90 commented May 17, 2020

I had to make a minor mod for it to work correctly in my scenario (inside a flex grow column height 100%): The viewportTopH does no need to include the preHeaderHeight.

Also made a change to take into account the showPreHeaderPanel value.

function resizeCanvas() {
      if (!initialized) { return; }
      paneTopH = 0;
      paneBottomH = 0;
      viewportTopH = 0;
      viewportBottomH = 0;
+      var preHeaderHeight = (options.createPreHeaderPanel && options.showPreHeaderPanel) ? (options.preHeaderPanelHeight || 0) : 0;

  //-- ... code removed for brevity --//

      // The top pane includes the top panel and the header row
+      paneTopH += topPanelH + preHeaderHeight + headerRowH + footerRowH;

      if (hasFrozenColumns() && options.autoHeight) {
        paneTopH += scrollbarDimensions.height;
      }

      // The top viewport does not contain the top panel or header row
      viewportTopH = paneTopH - topPanelH - headerRowH - footerRowH;

@ghiscoding
Copy link
Collaborator

Great thanks for the confirmation, I updated the PR #497 and tested locally too with the auto-resize example, removing that last line doesn't have any impact. Again thanks for confirming.

@6pac 6pac closed this as completed in af6151f May 17, 2020
@ghiscoding
Copy link
Collaborator

ghiscoding commented May 20, 2020

@xavier-90
So I'm not sure why you said that the 2nd line didn't need the preHeaderHeight in the calculation, it really does need it. I found out that adding it only to the first calculation broke my resize in Angular-Slickgrid. So I will have to put back 2nd line. A live demo here shows the new incorrect behavior.

New behavior

image

When I put back the calculation that I wrote earlier, it fixes my issue

// The top viewport does not contain the top panel or header row
+ viewportTopH = paneTopH - topPanelH - preHeaderHeight - headerRowH - footerRowH;

After that the grid resize is correct

image

... So I have no choice but to add that line back with preHeaderHeight in the calculation, (actually a rollback is what needs to be done), since I see that all grids that have a Pre-Header (dragging grouping, column grouping, ...) are now broken.

EDIT

Actually after investigating a bit further, the PR I made with the code change is completely invalid. resizeCanvas() already calls getViewportHeight() which itself also take the Pre-Header in consideration on this line. So I will actually rollback the entire thing. This is a major issue and I didn't find it earlier because there's no E2E tests to cover such UX issue, this one is an urgent rollback to deal with.

So to go back to your original question, it most probably is an issue in your own code, not in the SlickGrid itself. I will rollback PR PR #497, that was a bad code change from my side.

@xavier-90
Copy link
Author

I have done further tests and found that I was actually “barking at the wrong tree”. The issue is with the resizer: If setting the resizer bottom padding to 0 then the grid is rendered with a bottom padding of 20px (the default).
I have logged this as #504

@ghiscoding
Copy link
Collaborator

ok let's close this issue, the rollback PR was merged, let's discuss it further in the other issue #504

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

No branches or pull requests

2 participants