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.

Sign up for GitHub

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

Jump to bottom

[EuiDataGrid] How should the row height switcher toolbar control handle rowHeights overrides? #5411

Closed
cee-chen opened this issue Nov 24, 2021 · 8 comments
Closed

[EuiDataGrid] How should the row height switcher toolbar control handle rowHeights overrides? #5411

cee-chen opened this issue Nov 24, 2021 · 8 comments
Labels
data grid

Comments

@cee-chen
Copy link
Contributor

cee-chen commented Nov 24, 2021 •
edited
Loading

Should the row heights switcher (state set by the user):

  1. Override the rowHeights overrides (state set by the consumer)?

  2. Or, should the control be automatically disabled if rowHeights exists (we assume that the consumer does not want their overrides to be overridden)?

See #5372 (review)

[...] when the user selects an option, change it for all the rows even the very specifically set by index ones [or] If the setting seems to be completely controlled/set by the consumer, or a selection can't be determined, just hide it.
This is probably the easier option and probably aligns more with the consumer's intention. If they're explicitly setting row heights for individual rows, then they probably don't expect the content to fit any other way.

If a consumer has deliberately chose specific heights for specific rows, my guess is they don't want the consumer to adjust the row heights at all.
As a user, if I chose to specifically set the row heights to single, and saw that row 2, 4, and 5 were the only ones not adjusting, I'd think there was a bug in the UI. So I'm more trying to find a solution that aligns the consumer's intent with the user's expectation. We can't truly know both, but we can predict by defaulting to specific outcomes and providing an "out".
cee-chen added a commit to cee-chen/eui that referenced this issue Nov 24, 2021
@cee-chen
[PR feedback] Document recommendation for disabling row height switch…
4250a73 
…er when using `rowHeights`

@see elastic#5411
@cee-chen cee-chen mentioned this issue Nov 24, 2021
Merged
9 tasks
cee-chen pushed a commit that referenced this issue Nov 24, 2021
@cchaos
[EuiDataGrid] Add rowHeightSwitcher logic + API change (#5372)
ff742c6 
* Rename styleSelector to displaySelector + update types & docs

* Refactor out nested object helper

- so that both the columnSelector and displaySelector can use it

* Update displaySelector with conditional density/row height options

+ update docs:
  - reorder UI to match toolbar layout
  - fix legends

+ add unit tests (& improve some existing unit tests)

* Add rowHeightsOptions controls to popover

NYI: conditional lineCount controls

+ unit tests

* Add conditional lineCount row and logic
+ fix cell height to dynamically on lineCount change

* Fix styles not applying correctly for rowHeightsOptions that have undefined heights but defined rowHeightsOptions objects

e.g. - empty objs, or lineHeight set but nothing else

+ simplify this.props

* Fix multiline content not top-aligning correctly when in single/undefined mode

- Multiline content includes items with `<br>`/`<p>` tags etc that end up incorrectly vertically centered to their increased height

* Fix single/undefined row heights to account for passed lineHeight API + density

- by giving it the same setRowHeight logic as line counts with a single line

+ rename recalculateLineCountHeight for clarity now that it's no longer being used for just lineCount

* Add changelog entry

* [PR feedback] Increase default lineCount to 2

- to make the difference more obvious when switching between single and custom

* Fix changelog

* Fix control column appearance by switching them back to vertical centering

* Tweak various height alignments on compressed grid settings

- expand buttons on single lines should now be more vertically aligned

- The manual button/checkbox changes for control columns is somewhat required for auto height to not be dramatically different for single lines

* Fix expand action button background color mismatch for auto/lineCount rows
- when hovering over rows
- on striped rows

- Not caused by this PR, but this was really annoying me, haha

* PR feedback: convert cell actions CSS selectors to mixin

- Slightly less grody to look at in-code, and allows us to provide more comments for context

The more I look at __expandButton the more I dislike the naming also but it would involve too many changes right now to rename button->actions, so I added a TODO

* Fix sasslint issues

* PR feedback: Change line count number to EuiRange

+ Remove ability to set negative or zero numbers, which causes the grid to flip out

* PR feedback - remove height tweaks for compressed auto fit

* [PR feedback] Remove `showStyleSelector`

* Remove unused unit tests

- This should have been done in 79c878d, but I missed it

* [PR feedback] Intelligently disable toolbar control if all nested options are disabled

Notes:
- Strongly recommend turning off whitespace changes to reduce indentation noise
- This could have been done at the `data_grid_toolbar` level and could be a microperf optimization to do so, but since this is a relatively unlikely edge case in any case I don't think it's worth the overoptimization when this approach is relatively straightforward to implement

* Do not fall back to undefined/single for static heights

* PR changelog feedback

Co-authored-by: Caroline Horn <[email protected]>

* Update grid density to also intelligently detect initial state

+ not set a button state if custom fontSize/cellPadding don't match our defined density states

* [PR feedback] Improve typing to avoid any usage

+ improve unit tests to cover undefined nested object key case

* [PR feedback] Document recommendation for disabling row height switcher when using `rowHeights`

@see #5411

* [Pr feedback] Simplify getNestedObjectOptions

* Add workaround for failing Cypress tests

Also see https://stackoverflow.com/questions/49384120/resizeobserver-loop-limit-exceeded

Co-authored-by: Caroline Horn <[email protected]>
@cee-chen
Copy link
Contributor Author

CC @ryankeairns - any thoughts here? Does the Discover team have any plans on using both the new row height switcher and rowHeights overrides at the same time?

@cee-chen cee-chen mentioned this issue Nov 24, 2021
Merged
10 tasks
@elizabetdev
Copy link
Contributor

@constancecchen here's my design ideia.

I’m more leaning into option 2. But I don’t see any problem with also allowing option 1.

By default, if a consumer passed rowHeights, the row height switcher toolbar would be disabled (option 2). But we can also allow consumers to override this (option 1).

We can assume that the consumer does not want their overrides to be overridden but there are some scenarios that users might want to override that. So in case, a consumer passed a rowHeights configuration we can give the option to override the default rowHeights configuration:

Screenshot 2021-11-25 at 13 15 42

  • By default, if rowHeights exists the control is automatically disabled and an "Override default row height" switch appear.
  • Users can override that by switching on "Override default row height". Then when users select an option, all rows are overridden, even the very specific set by index ones.
  • By switching off "Override default row height", the state goes back to the rowHeights configuration set by the consumer.

Let me know what y'all think! 🧐

@ryankeairns
Copy link
Contributor

As I've always understood the Discover use case, the consumer would set a default (i.e. suggested) row height and the user would be able to change it to their personal preference. The switch to 'override the default row height' feels unexpected to me.

If the consumer does not want the default to be overridden, then I would not expect to see any UI for Row height. In that way, perhaps it is a feature that the consumer chooses to expose/display or not.

@cee-chen
Copy link
Contributor Author

cee-chen commented Nov 29, 2021 •
edited
Loading

Sorry to be clear, the switcher always overrides defaultHeight. This issue is about overriding specific rows heights, which is set by the rowHeights object config when the consumer wants (e.g.) row 2 to be 150px high (specific) but everything else to be 30px (default).

So in Elizabet's mocks should likely say "Override all row heights" not "Override default row height".

I think that switch works, but to be honest I'm more of a fan of reducing than I am of adding features for the sake of it. I'd be curious to hear what teams/consumers are even using the rowHeights config and why - I'm actually not even seeing any current usage in Kibana, and I'm curious who we implemented that for in the first place. To be honest, I think the row height switcher really negates most of the need for row-specific heights (since content and data is dynamic and going to display differently on each user's screen, presumably the user knows best what to set their row heights to).

@chandlerprall, do you have any context on where the impetus for rowHeights came from?

@ryankeairns
Copy link
Contributor

Thanks for clarifying. Yeah, I don't know of any specific use cases requiring the per-row settings and - barring any findings by others - would be in favor of minimizing further iteration given it's seemingly less common fit.

@cee-chen
Copy link
Contributor Author

Just chatted a bit more with Chandler on rowHeights and found the reasoning for it! In the future, we plan to let users vertically drag to expand/contract individual rows to set their heights the same way we let users horizontally drag to expand/contract column widths. rowHeights is the API/object structure we will likely use at that point.

That upcoming functionality does raise a great question for the row height switcher: when individual rows have customizable/draggable heights, how should this row height switcher handle that? IMO, I still don't think it makes sense for the defaultHeight to override individual row height settings, but is there any way to make it clear that a specific row has a custom height set on it? And do we need to we make it clear on this toolbar control that it affects default heights only?

cc @cchaos into this convo as well!

@cchaos
Copy link
Contributor

cchaos commented Nov 30, 2021

When trying to make a decision like this, I think about the most possible user flows. In this case there are two starting points, so here are what I consider the two main possible workflows.

  1. Developer initiated default row heights.
    1. User lands on the page with the DataGrid where the grid has been specifically laid out based on the contents.
    2. User has a hard time reading the small text, so they increase the grid settings to "Expanded".
    3. User sees an option for row height adjustment including "Auto fit" and "Single"
    4. User expects clicking this will ensure all content is revealed or every line changes to only a single line visible
  2. User adjust individual row heights.
    1. The reason they did so is because the content was not fully visible on the rows they wanted to see.
    2. Clicking around the grid interface, they find the display settings popover
    3. User sees an option for row height adjustment including "Auto fit" and "Single"
    4. User expects clicking this will ensure all content is revealed or every line changes to only a single line visible

Both user flows expect the same outcome which, in my mind, means that this is the optimal path. I'd truly advise against having to add another toggle just to enable/disable other controls. The UI can be "smart" enough to assume intent while also providing an "out". Which I would then say we could provide a button in the popover's footer to "Reset to defaults". This would return the grid to the consumer/engineer provided defaults (including density and any future display controls) without having to refresh the page.

So my suggestion summed up as tasks:

  • Density and row height controls always override engineer specifications if they're available to the user
  • Add a Reset to default button in the display controls popover that undos any changes the user made to the display selections
  • If the engineer does not want them to be overridden, they have to hide the control completely (this is content specific)

@cee-chen cee-chen mentioned this issue Dec 1, 2021
Merged
4 tasks
@cee-chen
Copy link
Contributor Author

cee-chen commented Dec 3, 2021

Closed by #5428

@cee-chen cee-chen closed this as completed Dec 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
data grid
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@ryankeairns @cee-chen @cchaos @elizabetdev