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

[EuiBasicTable] Option to make selected table items controlled from a parent component #6184

Closed
xcrzx opened this issue Aug 26, 2022 · 8 comments · Fixed by #7321
Closed
Assignees
Labels
feature request help wanted The EUI team is looking for community members to pick up and implement this issue

Comments

@xcrzx
Copy link

xcrzx commented Aug 26, 2022

Related issues: elastic/kibana#136616

Summary

Currently, EuiBasicTable doesn't support the table selection to be controlled from a parent component. That means if there's a global state that keeps track of selected table rows, all changes to that global state and the local EUI table state should be synchronized manually using the imperative setSelection() API. That makes state synchronization logic pretty complicated and error-prone. For example, here's how the selection state gets synchronized in Security Solution.

It would be nice to have an option to fully control selected table rows from the outside in a declarative way. Something like:

<EuiBasicTable
  selection={{
    selected: selectedItems
  }}
/>
@daveyholler
Copy link
Contributor

@xcrzx Sorry this took a minute... We're getting around to prioritizing this. Can you confirm that this is still a valid/needed request?

@xcrzx
Copy link
Author

xcrzx commented Dec 6, 2022

@xcrzx Sorry this took a minute... We're getting around to prioritizing this. Can you confirm that this is still a valid/needed request?

@daveyholler Yes, we still use a pretty cumbersome workaround

https://github.com/elastic/kibana/blob/ed1db36df7ff76c7ce54954af679749b09184576/x-pack/plugins/security_solution/public/detection_engine/rule_management_ui/components/rules_table/rules_tables.tsx#L172-L208

@cee-chen
Copy link
Member

cee-chen commented Jan 19, 2023

Jumping in here: using a ref to access internal methods isn't necessarily a workaround - we have publicly documented refs as a way of allowing access to/controlling internal state, such as in the case of EuiDataGrid (https://elastic.github.io/eui/#/tabular-content/data-grid-advanced#ref-methods).

While we don't publicly document this for EuiBasicTable, as it's a fairly edge case workaround, we do have an example of a controlled selection via tableRef in our own docs, and thus I would assume its usage is kosher.

I don't have super incredibly strong feelings one way or another, and I definitely see your use-case - just wanted to add the above note for help with prioritization.

@xcrzx
Copy link
Author

xcrzx commented Jan 23, 2023

Hey @cee-chen, thanks for joining the discussion. By workaround, I mean not the usage of the tableRef per se but the imperative nature of the setSelection() method. Instead of declaratively passing an array of selected items, we had to implement a pretty complex synchronization logic of the parent state and the table state. See the following code for example (full version here):

  const isSelectAllCalled = useRef(false);

  // TODO Remove this synchronization logic after https://github.com/elastic/eui/issues/6184 is implemented
  // Synchronize selectedRuleIds with EuiBasicTable's selected rows
  useValueChanged((ruleIds) => {
    if (tableRef.current != null) {
      tableRef.current.setSelection(rules.filter((rule) => ruleIds.includes(rule.id)));
    }
  }, selectedRuleIds);

  const euiBasicTableSelectionProps = useMemo(
    () => ({
      selectable: (item: Rule) => !loadingRuleIds.includes(item.id),
      onSelectionChange: (selected: Rule[]) => {
        /**
         * EuiBasicTable doesn't provide declarative API to control selected rows.
         * This limitation requires us to synchronize selection state manually using setSelection().
         * But it creates a chain reaction when the user clicks Select All:
         * selectAll() -> setSelection() -> onSelectionChange() -> setSelection().
         * To break the chain we should check whether the onSelectionChange was triggered
         * by the Select All action or not.
         *
         */
        if (isSelectAllCalled.current) {
          isSelectAllCalled.current = false;
          // Handle special case of unselecting all rules via checkbox
          // after all rules were selected via Bulk select.
          if (selected.length === 0) {
            setIsAllSelected(false);
            setSelectedRuleIds([]);
          }
        } else {
          setSelectedRuleIds(selected.map(({ id }) => id));
          setIsAllSelected(false);
        }
      },
    }),
    [loadingRuleIds, setIsAllSelected, setSelectedRuleIds]
  );

And even with that logic, we still don't cover all edge cases. See this issue, for example.

@cee-chen
Copy link
Member

Gotcha - that's super helpful context, thank you! I definitely see what you're saying / the argument for a first-party selected API!

@github-actions
Copy link

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

@cee-chen
Copy link
Member

@xcrzx As a heads up, this is finally coming to EUI :) Once #7321 lands in Kibana main, you'll be able to control table selection by passing <EuiBasicTable selection={{ selected }} />. Thanks for your patience with us as we work our way through our backlog!

@xcrzx
Copy link
Author

xcrzx commented Nov 1, 2023

Thank you @cee-chen 🙏
Added a ticket to migrate our code to the new API: elastic/kibana#170303

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request help wanted The EUI team is looking for community members to pick up and implement this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants