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

[EuiTable EuiBasicTable] Fix nested content alignment when selection is enabled #7895

Merged
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
6 changes: 6 additions & 0 deletions packages/eui/changelogs/upcoming/7895.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
**Bug fixes**

- Fixed overlapping content in `EuiBasicTable` for expanded and selectable table rows
- Added props `isExandedRow` and `hasCheckboxOffset` on `EuiTableRowCell`
- Fixed the alignment of `EuiBasicTable` mobile actions

Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { action } from '@storybook/addon-actions';
import { faker } from '@faker-js/faker';
import { moveStorybookControlsToCategory } from '../../../.storybook/utils';

import { useEuiTheme } from '../../services';
import { EuiLink } from '../link';
import { EuiHealth } from '../health';

Expand All @@ -20,6 +21,7 @@ import type {
EuiBasicTableColumn,
} from './basic_table';
import { EuiBasicTable, EuiBasicTableProps } from './basic_table';
import { EuiIcon } from '../icon';

// Set static seed so that the generated faker data is consistent between page loads
faker.seed(8_02_2010);
Expand Down Expand Up @@ -148,7 +150,7 @@ const columns: Array<EuiBasicTableColumn<User>> = [
'data-test-subj': 'action-outboundlink',
},
{
name: <>Clone</>,
name: 'Clone',
description: 'Clone this user',
icon: 'copy',
type: 'icon',
Expand Down Expand Up @@ -263,6 +265,61 @@ export const ExpandedRow: Story = {
},
};

const NestedTable = ({
hasLeadingIcon = false,
}: {
hasLeadingIcon?: boolean;
}) => {
const { euiTheme } = useEuiTheme();

const _items = users.slice(0, 3);
const _columns = hasLeadingIcon
? [
{
name: '',
render: () => <EuiIcon type="warning" />,
width: euiTheme.size.xl,
},
...columns,
]
: columns;

return (
<EuiBasicTable
tableCaption="EuiBasicTable playground"
items={_items}
itemId="id"
rowHeader="firstName"
columns={_columns}
/>
);
};

export const ExpandedNestedTable: Story = {
parameters: {
controls: {
include: ['columns', 'items', 'itemIdToExpandedRowMap'],
},
},
args: {
tableCaption: 'EuiBasicTable playground',
items: users,
itemId: 'id',
rowHeader: 'firstName',
columns,
itemIdToExpandedRowMap: {
1: <NestedTable />,
3: <NestedTable hasLeadingIcon />,
},
selection: {
selectable: (user) => user.online,
selectableMessage: (selectable) =>
!selectable ? 'User is currently offline' : '',
onSelectionChange: action('onSelectionChange'),
},
},
};

const StatefulPlayground = ({
items,
pagination,
Expand Down
7 changes: 6 additions & 1 deletion packages/eui/src/components/basic_table/basic_table.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1055,7 +1055,12 @@ export class EuiBasicTable<T extends object = any> extends Component<
isExpandedRow={true}
hasSelection={!!selection}
>
<EuiTableRowCell colSpan={expandedRowColSpan} textOnly={false}>
<EuiTableRowCell
colSpan={expandedRowColSpan}
textOnly={false}
isExpandedCell={true}
hasCheckboxOffset={!!selection}
>
{itemIdToExpandedRowMap![itemId]}
</EuiTableRowCell>
</EuiTableRow>
Expand Down
11 changes: 2 additions & 9 deletions packages/eui/src/components/table/table_row.styles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export const euiTableRowStyles = (euiThemeContext: UseEuiTheme) => {
const rowColors = _rowColorVariables(euiThemeContext);
const expandedAnimationCss = _expandedRowAnimation(euiThemeContext);

const { cellContentPadding, mobileSizes, checkboxSize } =
const { cellContentPadding, mobileSizes } =
euiTableVariables(euiThemeContext);

return {
Expand Down Expand Up @@ -61,13 +61,6 @@ export const euiTableRowStyles = (euiThemeContext: UseEuiTheme) => {
background-color: ${rowColors.selected.hover};
}
`,
// Offset expanded & selectable rows by the checkbox width to line up content with the 2nd column
// Set on the `<td>` because padding can't be applied to `<tr>` elements directly
checkboxOffset: css`
.euiTableRowCell:first-child {
Copy link
Contributor

@cee-chen cee-chen Jul 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm probably being a little nit-picky here, but wouldn't changing this to & > .euiTableRowCell:first-child { also have fixed the issue? I'm a little hesitant to apply new props to EuiTableRowCell mostly because it's a public top-level API change 🤷 (which would mean breaking changes or deprecations if they ever change again in the future)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh damn, yes you're absolutely right! I should have noticed that 🤦‍♀️
That is definitely a more straight forward solution. I'll update 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted the API changes here in favor of the suggested style solution.

${logicalCSS('padding-left', checkboxSize)}
}
`,
},

mobile: {
Expand Down Expand Up @@ -124,7 +117,7 @@ export const euiTableRowStyles = (euiThemeContext: UseEuiTheme) => {
${logicalCSS('border-top-left-radius', 0)}
${logicalCSS('border-top-right-radius', 0)}

.euiTableRowCell {
> .euiTableRowCell {
${logicalCSS('width', '100%')}
}

Expand Down
1 change: 0 additions & 1 deletion packages/eui/src/components/table/table_row.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@ export const EuiTableRow: FunctionComponent<Props> = ({
isSelected && styles.desktop.selected,
isExpandedRow && styles.desktop.expanded,
onClick && styles.desktop.clickable,
isExpandedRow && hasSelection && styles.desktop.checkboxOffset,
];

const classes = classNames('euiTableRow', className, {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,9 @@ const meta: Meta<EuiTableRowCellProps> = {
<EuiTable>
<EuiTableBody>
<EuiTableRow hasActions={args.hasActions}>
<EuiTableRowCell>Cell 1</EuiTableRowCell>
{(args.hasActions || args.isExpander) && (
<EuiTableRowCell>Cell 1</EuiTableRowCell>
)}
<Story {...args} />
</EuiTableRow>
</EuiTableBody>
Expand All @@ -48,6 +50,8 @@ const meta: Meta<EuiTableRowCellProps> = {
align: LEFT_ALIGNMENT,
hasActions: false,
isExpander: false,
isExpandedCell: false,
hasCheckboxOffset: false,
setScopeRow: false,
textOnly: true,
truncateText: false,
Expand Down
5 changes: 4 additions & 1 deletion packages/eui/src/components/table/table_row_cell.styles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { euiTableVariables } from './table.styles';
export const euiTableRowCellStyles = (euiThemeContext: UseEuiTheme) => {
const { euiTheme } = euiThemeContext;

const { mobileSizes } = euiTableVariables(euiThemeContext);
const { mobileSizes, checkboxSize } = euiTableVariables(euiThemeContext);

// Unsets the extra strut caused by inline-block display of buttons/icons/tooltips.
// Without this, the row height jumps whenever actions are disabled.
Expand Down Expand Up @@ -57,6 +57,9 @@ export const euiTableRowCellStyles = (euiThemeContext: UseEuiTheme) => {
bottom: css`
vertical-align: bottom;
`,
checkboxOffset: css`
${logicalCSS('padding-left', checkboxSize)}
`,

desktop: {
desktop: css`
Expand Down
13 changes: 13 additions & 0 deletions packages/eui/src/components/table/table_row_cell.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,16 @@ export interface EuiTableRowCellProps extends EuiTableRowCellSharedPropsShape {
* Indicates if the column is dedicated as the expandable row toggle
*/
isExpander?: boolean;
/**
* Indicates that it's a cell within an expanded row
*/
isExpandedCell?: boolean;
/**
* Apply to offset expanded & selectable rows by the
* checkbox width to line up content with the 2nd column.
* Only takes effect when isExpandedCell={true}
*/
hasCheckboxOffset?: boolean;
/**
* Mobile options for displaying differently at small screens;
* See #EuiTableRowCellMobileOptionsShape
Expand All @@ -125,6 +135,8 @@ export const EuiTableRowCell: FunctionComponent<Props> = ({
width,
valign = 'middle',
mobileOptions,
isExpandedCell,
hasCheckboxOffset,
...rest
}) => {
const isResponsive = useEuiTableIsResponsive();
Expand All @@ -134,6 +146,7 @@ export const EuiTableRowCell: FunctionComponent<Props> = ({
setScopeRow && styles.rowHeader,
isExpander && styles.isExpander,
hasActions && styles.hasActions,
isExpandedCell && hasCheckboxOffset && styles.checkboxOffset,
styles[valign],
...(isResponsive
? [
Expand Down
Loading