Skip to content

Commit

Permalink
πŸ› Fix custom action mobile styling
Browse files Browse the repository at this point in the history
- Fix 1: `flex-direction: column` and `padding: 0` was being applied to custom action cells when they should not have been

- Fix 2: table rows with only custom actions were still showing an empty right column border/padding on mobile. We needed the `actions="custom"` logic on table rows as well as row cells

+ Add a few test assertions to try and capture regressions in the future
  • Loading branch information
cee-chen committed Apr 2, 2024
1 parent 39c965b commit 4b89eaf
Show file tree
Hide file tree
Showing 5 changed files with 59 additions and 15 deletions.
24 changes: 24 additions & 0 deletions src/components/basic_table/__snapshots__/basic_table.test.tsx.snap
Original file line number Diff line number Diff line change
@@ -1,5 +1,29 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`EuiBasicTable actions custom item actions 1`] = `
<tr
class="euiTableRow euiTableRow-isSelectable euiTableRow-hasActions emotion-euiTableRow-mobile"
>
<td
class="euiTableRowCell euiTableRowCell--hasActions emotion-euiTableRowCell-hasActions-middle-mobile-customActions"
>
<div
class="euiTableCellContent emotion-euiTableCellContent-wrapText-actions"
>
<div
class=""
>
<button
data-test-subj="customAction-1"
>
Custom action
</button>
</div>
</div>
</td>
</tr>
`;

exports[`EuiBasicTable renders (bare-bones) 1`] = `
<div
aria-label="aria-label"
Expand Down
13 changes: 12 additions & 1 deletion src/components/basic_table/basic_table.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -704,7 +704,6 @@ describe('EuiBasicTable', () => {
const props: EuiBasicTableProps<BasicItem> = {
items: basicItems,
columns: [
...basicColumns,
{
name: 'Actions',
actions: [
Expand All @@ -729,9 +728,21 @@ describe('EuiBasicTable', () => {
expect(queryByTestSubject('customAction-2')).toBeInTheDocument();
expect(queryByTestSubject('customAction-3')).not.toBeInTheDocument();

// TODO: These assertions should ideally be visual regression snapshots instead
expect(
container.querySelector('.euiTableRowCell--hasActions')!.className
).toContain('-customActions');
expect(
container.querySelector(
'.euiTableRowCell--hasActions .euiTableCellContent'
)!.className
).not.toContain('-actions-mobile');
expect(
container.querySelector('.euiTableRow-hasActions')!.className
).not.toContain('-hasRightColumn');
expect(
container.querySelector('.euiTableRow-hasActions')
).toMatchSnapshot();
});

describe('are disabled on selection', () => {
Expand Down
23 changes: 14 additions & 9 deletions src/components/basic_table/basic_table.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -987,18 +987,26 @@ export class EuiBasicTable<T extends object = any> extends Component<
rowSelectionDisabled = !!isDisabled;
}

let hasActions;
let hasActions: 'custom' | boolean = false;
columns.forEach((column: EuiBasicTableColumn<T>, columnIndex: number) => {
if ((column as EuiTableActionsColumnType<T>).actions) {
const columnActions = (column as EuiTableActionsColumnType<T>).actions;

if (columnActions) {
const hasCustomActions = columnActions.some(
(action) => !!(action as CustomItemAction<T>).render
);
cells.push(
this.renderItemActionsCell(
itemId,
item,
column as EuiTableActionsColumnType<T>,
columnIndex
columnIndex,
hasCustomActions
)
);
hasActions = true;
// A table theoretically could have both custom and default action items
// If it has both, default action mobile row styles take precedence over custom
hasActions = !hasActions && hasCustomActions ? 'custom' : true;
} else if ((column as EuiTableFieldDataColumnType<T>).field) {
const fieldDataColumn = column as EuiTableFieldDataColumnType<T>;
cells.push(
Expand Down Expand Up @@ -1122,15 +1130,12 @@ export class EuiBasicTable<T extends object = any> extends Component<
itemId: ItemIdResolved,
item: T,
column: EuiTableActionsColumnType<T>,
columnIndex: number
columnIndex: number,
hasCustomActions: boolean
) {
// Disable all actions if any row(s) are selected
const allDisabled = this.state.selection.length > 0;

const hasCustomActions = column.actions.some(
(action: Action<T>) => !!(action as CustomItemAction<T>).render
);

let actualActions = column.actions.filter(
(action: Action<T>) => !action.available || action.available(item)
);
Expand Down
10 changes: 7 additions & 3 deletions src/components/table/_table_cell_content.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,13 @@ export const EuiTableCellContent: FunctionComponent<
!isResponsive && styles[align], // On mobile, always align cells to the left
truncateText === true && styles.truncateText,
truncateText === false && styles.wrapText,
hasActions && styles.hasActions.actions,
hasActions &&
(isResponsive ? styles.hasActions.mobile : styles.hasActions.desktop),
...(hasActions
? [
styles.hasActions.actions,
!isResponsive && styles.hasActions.desktop,
isResponsive && hasActions !== 'custom' && styles.hasActions.mobile,
]
: []),
];

const classes = classNames('euiTableCellContent', className);
Expand Down
4 changes: 2 additions & 2 deletions src/components/table/table_row.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export interface EuiTableRowProps {
* Indicates if the table has a dedicated column for actions
* (used for mobile styling and desktop action hover behavior)
*/
hasActions?: boolean;
hasActions?: boolean | 'custom';
/**
* Indicates if the row will have an expanded row
*/
Expand Down Expand Up @@ -75,7 +75,7 @@ export const EuiTableRow: FunctionComponent<Props> = ({
styles.mobile.mobile,
isSelected && styles.mobile.selected,
isExpandedRow && styles.mobile.expanded,
(hasActions || isExpandable || isExpandedRow) &&
(hasActions === true || isExpandable || isExpandedRow) &&
styles.mobile.hasRightColumn,
hasSelection && styles.mobile.hasLeftColumn,
]
Expand Down

0 comments on commit 4b89eaf

Please sign in to comment.