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

Enhancements for DataTable #10353

Merged
merged 19 commits into from Aug 3, 2020
Merged

Enhancements for DataTable #10353

merged 19 commits into from Aug 3, 2020

Conversation

philippjfr
Copy link
Contributor

@philippjfr philippjfr commented Jul 29, 2020

A number of improvements for DataTable partially made possible by upgrading to latest SlickGrid, but also properly taking advantage of some of the column auto-sizing behaviors that were previously not applied.

  • Adds an autosize_mode property which replaces the fit_columns property

Will have to think of naming here, in JS they are defined as:

    "GridAutosizeColsMode": {
      None: 'NOA',
      LegacyOff: 'LOF',
      LegacyForceFit: 'LFF',
      IgnoreViewport: 'IGV',
      FitColsToViewport: 'FCV',
      FitViewportToCols: 'FVC'
    },

where LegacyOff and LegacyForceFit correspond to fit_columns=True/False. For now I've set fit_columns=None and only use the option when set. Currently naming is not great:

``"fit_columns"``
    Fit columns into the available area, potentially resulting in
    squashed or unreadable columns.

Screen Shot 2020-07-29 at 9 59 53 PM

``"fit_viewport"``
    Adjust viewport width to the automatically computed column widths.

Screen Shot 2020-07-29 at 10 00 09 PM

``"ignore_viewport"``
    Automatically compute column widths ignoring the viewport area (leaves part of header empty).

Screen Shot 2020-07-29 at 10 16 03 PM

``"off"``
    Disable autosizing behavior, equivalent to fit_columns=False
    (legacy mode).

Screen Shot 2020-07-29 at 10 16 26 PM

``"force_fit"``
    Whether columns should be fit to available width, equivalent
    to setting fit_columns=True (legacy mode).

Screen Shot 2020-07-29 at 10 16 38 PM

``"none"``
    Do not automatically compute column widths.

Screen Shot 2020-07-29 at 10 16 17 PM

  • Adds an auto_edit option as requested in DataTable autoEdit expose #6864
  • Adds freeze_columns which allows freezing the first N columns (i.e. make them fixed while all other columns scroll)
  • Adds freeze_rows which allows freezing the first or last (if negative) N rows
  • Improves the sizing behavior in general (this is still quite hacky)

@philippjfr philippjfr force-pushed the philippjfr/slickgrid_upgrade branch from 5b3f81c to 95be8e0 Compare Jul 29, 2020
@mattpap mattpap added this to the 2.2 milestone Jul 29, 2020
@philippjfr
Copy link
Contributor Author

philippjfr commented Jul 29, 2020

@mattpap @bryevdv @jbednar (Initial) review appreciated, particularly suggestions for better naming for the autosize_mode options would help.

@jbednar
Copy link
Contributor

jbednar commented Jul 29, 2020

In the photos above, I can't see any difference between autosize_mode="fit_columns", autosize_mode="off", and autosize_mode="force_fit". Would there be a difference in a smaller page or for a larger table, maybe?

@jbednar
Copy link
Contributor

jbednar commented Jul 29, 2020

I also can't see a use case for ignore_viewport; do we need that? From that example I don't see why someone would choose that, but maybe there's a different example. From what I can see, there are more options here than are actually useful, and so I'd vote to suppress some or most of them if that's true, and to have good examples showing how they differ if it's not.

@philippjfr
Copy link
Contributor Author

philippjfr commented Jul 29, 2020

There may still be issues with the options, you can try out the different options here: http://6pac.github.io/SlickGrid/examples/example-size-to-content.html

@philippjfr
Copy link
Contributor Author

philippjfr commented Jul 29, 2020

I'd also be in favor of dropping the ignore_viewport option. That then leaves the two legacy options, the two new options and the 'none' option. We should also work out if we want to keep or deprecate the fit_columns option.

@jbednar
Copy link
Contributor

jbednar commented Jul 29, 2020

I tried playing with that page and only managed to confuse myself. I think you should identify a minimal set of what seems to be useful options, then have examples showing clearly how those options differ from each other. From that I think we can pick good names.

@philippjfr
Copy link
Contributor Author

philippjfr commented Jul 29, 2020

I can't see any difference between autosize_mode="fit_columns", autosize_mode="off", and autosize_mode="force_fit"

Look more closely there's definitely a difference between fit_columns and the other two. But the two legacy options do seem identical, which is weird because fit_columns=False used to behave more like the 'none' case.

@jbednar
Copy link
Contributor

jbednar commented Jul 29, 2020

Ok, if I put them onscreen at once, I can see that fit_columns has a smaller size of column C. That's subtle, though, and it's hard to get my head around why that's meaningful. If it's sizing to the contents of C, why not to index? Or does index become very wide later on in the table? Anyway, no need to answer me; I'd rather you just select the ones that seem meaningfully different and useful to you, and present how those differ.

@philippjfr
Copy link
Contributor Author

philippjfr commented Jul 29, 2020

Based on what I've seen these options are useful:

  • fit_columns (FitColsToViewport): Tries to compute column widths based on the columns cell contents (doesn't always work well but likely to be better than equal splitting) and then pads until viewport is filled. Useful for filling some area while also taking into account cell contents.
  • fit_viewport (FitViewportToCols): Computes the column widths and then adjusts the viewport size to match the computed sizes, suffers from the same issues estimating the sizes but useful when trying to select width appropriate for the table contents.
  • force_fit (LegacyForceFit): Equally split columns given total width (total_width/number_of_cols). This is the backwards compatible option (the old default), has drawbacks since columns will get squeezed if width is too small.
  • none (None): Do not auto-compute widths (old behavior when fit_columns=False). This is useful when manually adjusting all column width.

The other two don't seem useful as far as I can tell.

@jbednar
Copy link
Contributor

jbednar commented Jul 29, 2020

Sounds good, and those names don't sound crazy.

@bryevdv
Copy link
Member

bryevdv commented Jul 29, 2020

Names seem fine to me.

As long as we are making DataTable changes, the more I think about it, I change my mind and really would like to try to make at least some very small visual enhancements for this release. E.g. just some random playing around for 2 minutes yields something much more close to "light and modern":

Screen Shot 2020-07-29 at 4 33 43 PM

Do you want to include that in this update work, or should we make a separate issue?

@philippjfr
Copy link
Contributor Author

philippjfr commented Jul 29, 2020

I'm happy to include it, there seem to be some minor alignment issues in your screenshot. Just push it to the PR and I'll tweak it further.

@bryevdv
Copy link
Member

bryevdv commented Jul 29, 2020

@philippjfr Absolutely I was just futzing around in the browser dev tools. The main things I think would be the lowest hanging fruit:

  • remove the 90's style background image on the header row (but it will need some tweaking after, that's where I stopped)
  • remove the horizontal lines between rows
  • lighten the vertical dotted lines

@philippjfr
Copy link
Contributor Author

philippjfr commented Jul 30, 2020

Okay I'm now happy with the sizing behavior and there are no awful hacks anymore. Here's my overview of the behavior for the four options:

fit_columns

Fixed width

Screen Shot 2020-07-30 at 4 49 36 PM

Responsive width

Screen Shot 2020-07-30 at 4 51 02 PM

Fixed width with explicit column widths

Screen Shot 2020-07-30 at 4 54 32 PM

fit_viewport

Screen Shot 2020-07-30 at 4 55 36 PM

*fit_viewport ignores responsive sizing and explicit columns width

force_fit

Fixed width

Screen Shot 2020-07-30 at 4 58 32 PM

Responsive width

Screen Shot 2020-07-30 at 4 58 45 PM

Fixed width with explicit column widths

force_fit ignores explicit column widths

none

Fixed width/Responsive width

Screen Shot 2020-07-30 at 5 00 30 PM

*none uses the explictly set widths and nothing else

Fixed width with explicit column widths

Screen Shot 2020-07-30 at 5 01 13 PM

@philippjfr
Copy link
Contributor Author

philippjfr commented Jul 30, 2020

@bryevdv @mattpap Would really appreciate it if you could point me to the types of tests you'd want me to add.

@bryevdv
Copy link
Member

bryevdv commented Jul 30, 2020

@philippjfr is widths new? If so it is awfully close to width, perhaps more explicit column_widths if that is an option

As for tests, I don't have much to say about testing layout, my main interest is that the existing integration tests all pass.

@philippjfr philippjfr force-pushed the philippjfr/slickgrid_upgrade branch from 8eb8fff to 1293213 Compare Aug 3, 2020
@philippjfr
Copy link
Contributor Author

philippjfr commented Aug 3, 2020

@philippjfr, this has to be rebased on top of the default branch, to clear unrelated test failures.

Done.

@mattpap
Copy link
Contributor

mattpap commented Aug 3, 2020

Wish I could test scrolling but I guess that'll have to wait.

Most likely, but I will see if can hack something together to just support this particular case.

@philippjfr
Copy link
Contributor Author

philippjfr commented Aug 3, 2020

Still seeing a small issue in one of the unit tests which I think is unrelated to my changes:

def test_create_chromium_webdriver() -> None:

Otherwise this is ready to review from my perspective. I'd like to do some more testing once a dev release is out.

@mattpap
Copy link
Contributor

mattpap commented Aug 3, 2020

Still seeing a small issue in one of the unit tests which I think is unrelated to my changes:

Webdriver tests fail regularly on windows and macos.

@philippjfr
Copy link
Contributor Author

philippjfr commented Aug 3, 2020

Missed on real failure in the integration tests. Looking at that now.

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

Successfully merging this pull request may close these issues.

[Feature request] Allow DataTable to take its natural height DataTable autoEdit expose
4 participants