eui

eui copied to clipboard

Summary of issue

If the columns array reference passed to EuiDataGrid ever changes, column order resets to the initial order. Since column reordering only tracks a list of column ids, if the set and order of column ids passed to the grid never changes, column ordering should be maintained across different column array references.

Instructions to reproduce

• Go to the codesandbox here

First, view the correct order behavior with static column reference

• Initial column order: "name", "age", "homeTown", "affiliation"
• Click the "name" column header and select move right
  • New column order: "age", "name", "homeTown", "affiliation"
• Click the "affiliation" column header and select move left
  • New column order: "age", "name", "affiliation", "homeTown"

Now view what happens when the reference changes

• Refresh the code sandbox to start from scratch
• Initial column order: "name", "age", "homeTown", "affiliation"
• Click the "name" column header and select move right
  • New column order: "age", "name", "homeTown", "affiliation"
• Now click the "Create new columns reference" button - This just changes a dependency of the columns useMemo for demonstration purposes to generate an identical array of column ids in the same order, but with a new outer reference
• Click the "affiliation" column header and select move left
  • New column order: "name", "age", "affiliation", "homeTown"
• When changing the reference, the column order tracked by EuiDataGrid internally was reset to the default order "name", "age", "homeTown", "affiliation", such that moving the "affiliation" column left applied to the original column order, rather than the current column order

Additional info

I think I have a clear idea of why this is happening. Column ordering is tracked by EuiDataGrid as depdendent state that depends on the columns passed as props. Any time columns change (even just the array reference), the dependent state triggers a reset of the column order. Ideally this dependent state would be smarter and check for column changes by seeing if the set and order of column ids has changed, rather than a simple comparison to the array reference.

I would be happy to work on this issue if others agree that this should be fixed.

Thank you for the issue @tgalfin. I've added it to the EUI team discussion this week. I'll update my comment after the team has discussed it.

While we may need to make this more obvious in our EuiDataGrid documentation, in general EUI strongly recommends that consumers ensure their columns definitions are either static (declared as consts outside of a React component is ideal) or memoized using useMemo with as few dependencies/redefinitions as possible.

While typically I would consider this to be an implementation issue that could be resolved by the consumer, I do see your point below:

Ideally this dependent state would be smarter and check for column changes by seeing if the set and order of column ids has changed, rather than a simple comparison to the array reference.

My only (very minor) concern with this approach is that it might be more of a perf hit to assume consumers have not correctly memoized their columns. This approach would require caching a 'previous' column ID array/set and comparing it to a new ordered ID set on every columns change vs the simpler and faster comparison.

That being said, EuiDataGrid is doing so many dang things and is something of a performance black hole in any case, so my vote is that if you can write the code elegantly enough (I'd probably suggest using a ref to store the previous comparison) and are willing to figure out writing a test for this use case (I can help out with this if Jest isn't getting you far enough and you have to use Cypress, LMK) - then we'd absolutely take a PR on this!

Thanks for your very thorough repro steps and description of the issue, as well as your willingness to contribute to EUI - it's seriously very appreciated! :heart:

That being said, EuiDataGrid is doing so many dang things and is something of a performance black hole in any case, so my vote is that if you can write the code elegantly enough (I'd probably suggest using a ref to store the previous comparison) and are willing to figure out writing a test for this use case (I can help out with this if Jest isn't getting you far enough and you have to use Cypress, LMK) - then we'd absolutely take a PR on this!

Thanks for the quick response on this. I can't promise the first try will be elegant, but I would like to get a working proof of concept first to make sure I can fix the issue in the right location without breaking stuff. If I get something working, can I open a draft PR and tag you? Once I get some initial feedback that I'm at least the right track, then I can try to clean it up and add tests.

Absolutely! Please feel free to tag me in a draft PR!

👋 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.

❌ Per our previous message, this issue is auto-closing after having been open and inactive for a year. If you strongly feel this is still a high-priority issue, or are interested in contributing, please leave a comment or open a new issue linking to this one for context.