eui

eui copied to clipboard

We recently implemented custom row height selection in Discover 🥳 (PR #122087) . There's an issue we've encountered. When users select a new row height, this is propagated to the URL, and EuiDataGrid retrieves this new setting. So it's the new default. But the way it currently works is that once users change the row height, the "Reset to default" link is displayed, and clicking on it makes it disappear.

https://user-images.githubusercontent.com/463851/152317459-3238c9f1-e812-4915-b66e-d2c3cde28850.mp4

What's more, if the user changes the setting in URL, it's ignored (given the value was changed in the popover before), so the external change doesn't change the grid's internal one.

But the way it currently works is that once users change the row height, the "Reset to default" link is displayed, and clicking on it makes it disappear.

Is this just an undesired behavior on your end? Our localStorage example does the same thing, and I guess I didn't really see that as a problem - the reset button allows users to set the grid back to the way it was when the page loaded, which seems like a valid use of the button. The user can just ignore it if they're fine with the changes, and the next time they visit the page their settings will be saved.

CCing @cchaos as she originally requested the button - should we consider passing in an option that allows applications to disable the Reset button on the display selector?

What's more, if the user changes the setting in URL, it's ignored

This confuses me a bit - if a user changes the setting in the URL, they're essentially reloading the page and causing a full data grid remount, correct? At which point there should no longer be internal user settings, and the settings loaded in from the consuming application should be the default. How are those changes being ignored then?

(Linking these two recent issues for historical context as they're somewhat related and have a caveat section on how user changes should override application changes - https://github.com/elastic/eui/pull/5526 and https://github.com/elastic/eui/issues/5524)

I also am confused by the settings in the url implementation in Discover. Wouldn't this make the browser history way too complex in terms of the user's understanding of how the back button works? It honestly doesn't seem like the right location to be storing these settings. They're not creating new pages but just altering the display.

I'll follow up here, after my vacation ... just wanted to express the joy, that with "Auto fit" we now can reveal unknown details of our content

https://user-images.githubusercontent.com/463851/152569722-8b55d6de-9e42-4d9a-8361-066487da9fd2.mp4

I won't lie, the timing on that auto fit reveal absolutely slayed me

We absolutely need this as a demo! 😹

Will ask the cat for permission (which reminds me, I think I didn't ask, but bribed with some good bites) 😸

Anyway, let me show what I mean. Changing to 7

So the internal state of the popover should the changed when external props overwrite it's settings

This confuses me a bit - if a user changes the setting in the URL, they're essentially reloading the page and causing a full data grid remount, correct?

No there's a listener for URL changes modifying the state in Discover

I also am confused by the settings in the url implementation in Discover. Wouldn't this make the browser history way too complex in terms of the user's understanding of how the back button works? It honestly doesn't seem like the right location to be storing these settings. They're not creating new pages but just altering the display.

This is how it's historically done in Discover, most of the state is in URL. I can see your point about the browser history ... also reproduced it ... when switching the Lines per row slider, plenty of history entries are created ... think no history should be added in this case, the current history entry should be replaced

BTW, not only allows data grid to reveal unknown details of your content, it also helps to hide unwanted details, so you can focus of things that really matter:

https://user-images.githubusercontent.com/463851/156739681-d5cc9f6b-ef6e-4263-a3d6-18edc97a6626.mp4

Hey Matthias! Sorry, I didn't mean to let this drop to the floor or get distracted by your hilarious cat. I see now that you're storing display selector state in a URL param which wouldn't trigger a page rerender. I'm still a little confused as to why the Reset to default button is problematic in terms of UI. I still think it's valid for a user to reset to whatever settings they had on page load. If you simply don't want users to be able to reset, would a config option to disable the reset button suffice?

hi, sorry too, for using my distracting cat 😄 . So it's not problematic, it's just redundant. Users can't reset to the state on page load, because every change is propagated to external state and becomes the default state. But the popover doesn't know and therefore offers to reset to default state ... I'd suggest to have a quick zoom when it's convenient for you, if you want

I think we can discuss the discover setup with the first Data grid styling and control example as a proxy.

https://user-images.githubusercontent.com/313125/158444178-aea684a0-8a24-4ad1-839c-3eed721644b6.mov

The difference between the example & discover's implementation is discover uses gridStyle.onChange to merge the user's changes into their grid style object. To emulate this thought with the example, select 'normal' density through the grid control and then update the gridStyle options with medium padding & font size; now open the grid's control again, the ask here is that the Reset button would be gone because resetting would not effect any difference.

My 2 cents: the reset button's existence is based on if the user interacts with the grid control to change from the default, we may be able to make this respect changes to gridStyles as well.

My 2 cents with a Front End Engineer hat: the application should add a layer of state to hold the initial page load data (which would update if the user refreshes the page or otherwise loads from the stateful URL) separate from updated-but-not-committed state

My 2 cents with an Information Architecture or Product hat: the core issue is treating both ephemeral & permanent state the same. Even if we implement Front End Engineer's above suggestion, the user journey doesn't feel right because upon re-loading the page with the updated styles in the URL that becomes the new default, instead of continuing to allow the app to set defaults + user overrides, providing a predictive flow allowing users to always reset any changes.

My 2 cents with a Kibana Engineer hat: I sure do wish we had a mechanism to save per-user settings 😁

Huh, good points all those random ~people~ hats! @kertal & @constancecchen what do you think about the idea from "IA/Product"? (seems like the most complete solution to me)

Super sorry, maybe I'm just not familiar with Discover's end-use of data grid here but I'm not clear what specifically you're proposing with the "IA/Product" paragraph. Are you saying that the fix is on the Kibana/Discover side of things and not EUI?

I can def see something like passing an optional resetSelectorState fn to the onChange callbacks (which is what I think you proposed in one of the cents? Unless I totally misread?) - that feels like a fairly quick solution.

Are you saying that the fix is on the Kibana/Discover side of things and not EUI?

From my IA/Product hat persona, yes. Though I don't think that's a workable answer here.

I'd hesitate to put further onus on the application to know when to enable/disable the reset button; how much effort would it be to make the reset button compare the active styling (app+merged user selection) with the app-provided styling, and detect if the styles are resettable?

For example: if the app keeps a state of whether the user has overridden any styles, and when they do display a "save these settings" button that would, when clicked, copy the user's configuration to a state and pass that into the data grid. The grid would then compare the provided styles with the active, user-merged styles, and only show the reset button if they continue to differ.

The grid would then compare the provided styles with the active, user-merged styles, and only show the reset button if they continue to differ.

If we really think this is worth doing, it's doable. We can change the showResetButton logic to something like

const showResetButton = useMemo(() => {
  // Object.entries iterate through `initialStyles`, check that each key in the obj that exists in `userGridStyles` matches values, if no, then return true

  // Object.entries iterate through `initialRowHeightsOptions`, check that each key in the obj that exists in `userGridStyles` matches value, if no, then return true

  // return false
});

Do we really think that level of iteration is worth it for what is a very minor UI/UX effect however? 🤷 If so then sure, I can open a PR for this.

It does* align the actual functionality with the perceived state of things. Right now, if an application changes its grid styles for any reason we apply them (when not overridden) but don't handle that in the the reset button. I think it's a better fundamental change to make than provide a bandaid an application can call in those edge cases.

• @kertal can you add any thoughts on the proposed solution's ability to resolve the discover issue?

Let me know if UX needs to jump in on this issue. I've kind of lost track of what the problem/solution is.

@chandlerprall @chandlerprall @cchaos Yes, this would solve this issue in Discover, when changing the rowHeight, there's the reset button displayed, but clicking it just makes it disappear, so showing it in this case makes no sense (it's not a big thing, no high priority)

Do we really think that level of iteration is worth it for what is a very minor UI/UX effect however? 🤷 If so then sure, I can open a PR for this.

@constancecchen I think it is worth it, the computation cycle will be small and should be limited by useMemo; may not present the user with the best experience in this type of setup as they can't unset the app's config, but it's better than the buggy experience now.

👋 Hey there. This issue hasn't had any activity for 180 days. We'll automatically close it if that trend continues for another week. If you feel this issue is still valid and needs attention please let us know with a comment.

❌ We're automatically closing this issue due to lack of activity. Please comment if you feel this was done in error.

I tested it, and I still thing this is worth an improvement

Hey @kertal. We've been grooming our enormous backlog over the past couple of months, trying to sort it into a prioritized list. We've identified this as a "Low" priority, "Medium" effort task, which means, this is very unlikely to be prioritized by our team to address. We simply don't have the time or resources.

For reference: https://github.com/orgs/elastic/projects/1079

If you feel this warrants more than a "Low" priority, please let us know. We're also happy to accept pull requests!

thx @JasonStoltz I do agree it's a low priority task, sadly it's unlikely that there will be a pull request, for the same reason: time / resources

👋 Hi there - this issue hasn't had any activity in 6 months. If the EUI team has not explicitly expressed that this is something on our roadmap, it's unlikely that we'll pick this issue up. We would sincerely appreciate a PR/community contribution if this is something that matters to you! If not, and there is no further activity on this issue for another 6 months (i.e. it's stale for over a year), the issue will be auto-closed.

@kertal can we close this issue out now that your team has added a allowResetButton configuration that allows hiding the reset button completely?

yes we can! 👍