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

Added initialWidth and isResizable configurables to EuiDataGridColumn #2696

Merged

Conversation

chandlerprall
Copy link
Contributor

@chandlerprall chandlerprall commented Dec 18, 2019

Summary

Closes #2621

Adds two new properties to the data grid column configuration:

  • initialWidth sets a column's default width on initial load
  • isResizable prevents the column from being resized via the UI

Additionally, I fixed a bug where the default column width was not reset when a new column list is provided.

@snide I went back and forth on whether a non-resizable column should remove the drag handle completely (which is what I did) vs. leave the on-hover effect with a disabled cursor. What do you think?

Checklist

- [ ] Check against all themes for compatability in both light and dark modes
- [ ] Checked in mobile
- [ ] Checked in IE11 and Firefox

  • Props have proper autodocs
  • Added documentation examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@myasonik
Copy link
Contributor

Not @snide but want to chime in on the handle thing...

In an ideal world, I guess, it'd be configurable. Because, for Discover's use case, where I want to disable resizing on a column that I am creating (a column for actions), I think users don't expect to be able to resize it.

But, if I had some use case where I wanted to disable resizing on a user defined column, then I'd want a disabled cursor to give the user a bit more understanding into what's happening.

I started off by saying "in an ideal world" because yet another boolean might not be great at this point with a ballooning API surface area...

@myasonik
Copy link
Contributor

Oh, also had the thought that whatever we do here we should then probably mimic in the column header dropdown actions menu (#2461)...

Copy link
Contributor

@myasonik myasonik left a comment

Choose a reason for hiding this comment

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

Should also add to the "kitchen sink" example titled "Snippet with every feature in use"

@chandlerprall
Copy link
Contributor Author

Should also add to the "kitchen sink" example titled "Snippet with every feature in use"

which reminds me I didn't update the snippet I copied to begin with

@snide
Copy link
Contributor

snide commented Dec 18, 2019

I went back and forth on whether a non-resizable column should remove the drag handle completely (which is what I did) vs. leave the on-hover effect with a disabled cursor. What do you think?

I agree with removing it. It's always best to remove controls when you don't expect them to change. I can't think of many scenarios where this would toggle as you interacted with the application.

In an ideal world, I guess, it'd be configurable.

We can always move there later. It's best to start with the best, simplest experience for the user first, and then see if they react and need more features.

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

I tested this for functionality pretty rigoursly, mostly assuming that it would bust in the various scroll / full width scenarios, but it all works as I'd expect. I did not overly check the tests code, but at least intentionally busted something to make sure they ran correctly.

My only other main suggestion would be to use this feature on the default example is some way, since it seems like a pretty core feature that people would utilize. That along with the other small quibbles, and this looks good to me.

@chandlerprall
Copy link
Contributor Author

@snide I updated the snippet and applied the two new configurations to the main example's version column

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Double approved

@chandlerprall chandlerprall merged commit 0e50f8f into elastic:master Dec 19, 2019
@chandlerprall chandlerprall deleted the datagrid/2621-column-sizing branch December 19, 2019 18:16
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.

DataGrid needs more column width configs
3 participants