Default sortable to true for all table columns

[igoristic]

Context: elastic/kibana#35218

Seems like most of the time we want all table columns to be sortable by default.

I think it would be less tedious to explicitly turn off sorting for unsupported columns rather than always enabling it for all supported columns.

This is just something to ponder upon

[cchaos]

Changing the default from false to true would cause a breaking change. I'm wondering, however, if we could add a single prop to EuiBasicTable like enableSorting that when added would try to add the default sorting to all columns that allow it. So then it's one prop instead a prop times how many columns.

[Vineet-Sharma29]

Hi @cchaos I would like to work on this. Can you explain what should be done?

[shakti97]

Can I take this up?

[cchaos]

Hi @Vineet-Sharma29 & @shakti97 , as we are open source we absolutely invite anyone to take on issues from our backlog, no need to ask permission. Just create a PR and we'll be able to review and help with any problems.

As for the specific question of how to accomplish this task, the EuiBasicTable allows consumers to pass a data object with column specifications. Right now a consumer has to specify if they want each individual column to be sortable. The issue here is that can be cumbersome if there are lots of columns and you just want to use the default sorting ability.

If we then add a prop called something like enableSorting on the EuiBasicTable component itself, then in the logic where it sets up each column, it should set the sorting to true. This could get complicated though if their data isn't setup to match what the sorting method expects. I'd then lean on @chandlerprall or @thompsongl for help down that line.

[Vineet-Sharma29]

@cchaos Can you tell how to check changes made in the codebase?

[Vineet-Sharma29]

I am planning to make following changes:-
table/table.tsx

...
export interface EuiTableProps
  extends CommonProps,
    TableHTMLAttributes<HTMLTableElement> {
  compressed?: boolean;
  responsive?: boolean;
  /**
   * Sets the table-layout CSS property
   */
  tableLayout?: 'fixed' | 'auto';
  enableSorting?: boolean;
}

export const EuiTable: FunctionComponent<EuiTableProps> = ({
  children,
  className,
  compressed,
  tableLayout = 'fixed',
  responsive = true,
  enableSorting,
  ...rest
}) => {
  const classes = classNames(
    'euiTable',
    className,
    {
      'euiTable--compressed': compressed,
      'euiTable--responsive': responsive,
      'euiTable--enableSorting': enableSorting,
    },
    tableLayoutToClassMap[tableLayout]
  );

  return (
    <table className={classes} {...rest}>
      {children}
    </table>
  );
};
...

basic_table.tsx

...
static defaultProps = {
    responsive: true,
    enableSorting: false, /* make sorting all column option false by default */
    tableLayout: 'fixed',
    noItemsMessage: 'No items found',
  };
...
renderTableMobileSort() {
    const { columns, sorting, enableSorting } = this.props;
    const items: EuiTableSortMobileProps['items'] = [];

    if (!sorting) {
      return null;
    }

    /* if enableSorting is true then add sorting options to all columns */
    if (enableSorting) {
      columns.forEach((column: EuiBasicTableColumn<T>, index: number) => {
        const sortDirection = this.resolveColumnSortDirection(column);

        items.push({
          name: column.name,
          key: `_data_s_${
            (column as EuiTableFieldDataColumnType<T>).field
          }_${index}`,
          onSort: this.resolveColumnOnSort(column),
          isSorted: !!sortDirection,
          isSortAscending: sortDirection
            ? SortDirection.isAsc(sortDirection)
            : undefined,
        });
      });
    } else {
    /* else check for sortable for prop in each column */
      columns.forEach((column: EuiBasicTableColumn<T>, index: number) => {
        if (
          !(column as EuiTableFieldDataColumnType<T>).sortable ||
          (column as EuiTableFieldDataColumnType<T>).hideForMobile
        ) {
          return;
        }

        const sortDirection = this.resolveColumnSortDirection(column);

        items.push({
          name: column.name,
          key: `_data_s_${
            (column as EuiTableFieldDataColumnType<T>).field
          }_${index}`,
          onSort: this.resolveColumnOnSort(column),
          isSorted: !!sortDirection,
          isSortAscending: sortDirection
            ? SortDirection.isAsc(sortDirection)
            : undefined,
        });
      });
    }

    return items.length ? <EuiTableSortMobile items={items} /> : null;
  }
...
renderTableHead() {
    const { columns, selection } = this.props;

    const headers: ReactNode[] = [];

    if (selection) {
      headers.push(
        <EuiTableHeaderCellCheckbox key="_selection_column_h">
          {this.renderSelectAll(false)}
        </EuiTableHeaderCellCheckbox>
      );
    }

    columns.forEach((column: EuiBasicTableColumn<T>, index: number) => {
      const {
        field,
        width,
        name,
        align,
        dataType,
        sortable,
        mobileOptions,
        isMobileHeader,
        hideForMobile,
      } = column as EuiTableFieldDataColumnType<T>;

      const columnAlign = align || this.getAlignForDataType(dataType);

      // actions column
      if ((column as EuiTableActionsColumnType<T>).actions) {
        headers.push(
          <EuiTableHeaderCell
            key={`_actions_h_${index}`}
            align="right"
            width={width}
            mobileOptions={mobileOptions}>
            {name}
          </EuiTableHeaderCell>
        );
        return;
      }

      // computed column
      if (!(column as EuiTableFieldDataColumnType<T>).field) {
        const sorting: SortOptions = {};
        // computed columns are only sortable if their `sortable` is a function
        if (this.props.sorting && typeof sortable === 'function') {
          const sortDirection = this.resolveColumnSortDirection(column);
          sorting.isSorted = !!sortDirection;
          sorting.isSortAscending = sortDirection
            ? SortDirection.isAsc(sortDirection)
            : undefined;
          sorting.onSort = this.resolveColumnOnSort(column);
          sorting.allowNeutralSort = this.props.sorting.allowNeutralSort;
        }
        headers.push(
          <EuiTableHeaderCell
            key={`_computed_column_h_${index}`}
            align={columnAlign}
            width={width}
            mobileOptions={mobileOptions}
            data-test-subj={`tableHeaderCell_${name}_${index}`}
            {...sorting}>
            {name}
          </EuiTableHeaderCell>
        );
        return;
      }

      // field data column
      const sorting: SortOptions = {};


     /* An extra or condition with enableSorting is needed to add sorting when it is true */
      if (this.props.sorting && (sortable || this.props.enableSorting)) {
        const sortDirection = this.resolveColumnSortDirection(column);
        sorting.isSorted = !!sortDirection;
        sorting.isSortAscending = sortDirection
          ? SortDirection.isAsc(sortDirection)
          : undefined;
        sorting.onSort = this.resolveColumnOnSort(column);
        sorting.allowNeutralSort = this.props.sorting.allowNeutralSort;
      }
      headers.push(
        <EuiTableHeaderCell
          key={`_data_h_${field}_${index}`}
          align={columnAlign}
          width={width}
          isMobileHeader={isMobileHeader}
          hideForMobile={hideForMobile}
          mobileOptions={mobileOptions}
          data-test-subj={`tableHeaderCell_${field}_${index}`}
          {...sorting}>
          {name}
        </EuiTableHeaderCell>
      );
    });

    return <EuiTableHeader>{headers}</EuiTableHeader>;
  }
...
resolveColumnSortDirection = (column: EuiBasicTableColumn<T>) => {
    const { sorting, enableSorting } = this.props;
    const { sortable, field, name } = column as EuiTableFieldDataColumnType<T>;
  
   /* if enable sorting is false then also return back */

    if (!sorting || !sorting.sort || !sortable || !enableSorting) {
      return;
    }
    if (sorting.sort.field === field || sorting.sort.field === name) {
      return sorting.sort.direction;
    }
  };
...
resolveColumnOnSort = (column: EuiBasicTableColumn<T>) => {
    const { sorting, enableSorting } = this.props;
    const { sortable, name } = column as EuiTableFieldDataColumnType<T>;

   /* if enable sorting is false then also return back */
   
    if (!sorting || !sortable || !enableSorting) {
      return;
    }
    if (!this.props.onChange) {
      throw new Error(`BasicTable is configured to be sortable on column [${name}] but
          [onChange] is not configured. This callback must be implemented to handle the sort requests`);
    }
    return () => this.onColumnSortChange(column);
  };
...

I am planning to make above changes. Can someone @cchaos , @chandlerprall , @thompsongl once guide check this ? Need some guidance with this issue

[cchaos]

Hi @Vineet-Sharma29, we use the standard fork & pull request workflow. You can find a good intro and walkthrough of the process here: https://gist.github.com/Chaser324/ce0505fbed06b947d962

However, it looks like someone else has already created a PR #2906 to fix this issue.

[Vineet-Sharma29]

@cchaos ok

[Vineet-Sharma29]

@cchaos Also, if possible can you please consider assigning issues since it helps in working on one. If someone worked on a issue but some other interested contributor filed a PR early, which means first contributor's work got wasted.

[cchaos]

@Vineet-Sharma29 I understand that can be frustrating, however, we don't like to assign issues to community members as we don't want to put pressure on those individuals. They are very welcome to self-assign themselves to issues that they will be working on. Your best bet is to subscribe to the issue to get updates. Unfortunately we cannot control who submits a PR first.