-
Notifications
You must be signed in to change notification settings - Fork 842
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
[EuiDataGrid] Reset internal state of row height / density popover when props change #5593
Comments
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
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 - #5526 and #5524) |
I also am confused by the |
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 Auto-fit-cat.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) 😸 |
No there's a listener for URL changes modifying the state in Discover |
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: karli-grumpy.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 |
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. datagrid-gridstyle-reset.movThe difference between the example & discover's implementation is discover uses 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 |
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 |
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. |
If we really think this is worth doing, it's doable. We can change the
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.
|
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 |
@constancecchen I think it is worth it, the computation cycle will be small and should be limited by |
👋 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 |
yes we can! 👍 |
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.reset-to-default.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.
The text was updated successfully, but these errors were encountered: