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.

Sign up for GitHub

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

[ML] Fixes responsive layout for Trained Models table #181541

Merged
merged 4 commits into from
Apr 24, 2024
Merged

[ML] Fixes responsive layout for Trained Models table #181541

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e443500
update columns width, set responsiveBreakpoint
darnautov Apr 24, 2024
5776e5b
fix deploy button for smaller screens
darnautov Apr 24, 2024
e736905
responsive deployment stats
darnautov Apr 24, 2024
9436c2f
download action as icon
darnautov Apr 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions x-pack/plugins/ml/public/application/model_management/model_actions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,9 +199,8 @@ export function useModelActions({
}
),
'data-test-subj': 'mlModelsTableRowStartDeploymentAction',
// @ts-ignore EUI has a type check issue when type "button" is combined with an icon.
icon: 'play',
type: 'button',
type: 'icon',
isPrimary: true,
enabled: (item) => {
return canStartStopTrainedModels && !isLoading && item.state !== MODEL_STATE.DOWNLOADING;
Expand Down
15 changes: 7 additions & 8 deletions x-pack/plugins/ml/public/application/model_management/models_list.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,7 @@ export const ModelsList: FC<Props> = ({
const columns: Array<EuiBasicTableColumn<ModelItem>> = [
{
align: 'left',
width: '40px',
width: '2%',
isExpander: true,
render: (item: ModelItem) => {
if (!item.stats) {
Expand All @@ -512,7 +512,7 @@ export const ModelsList: FC<Props> = ({
},
{
name: modelIdColumnName,
width: '215px',
width: '15%',
sortable: ({ model_id: modelId }: ModelItem) => modelId,
truncateText: false,
textOnly: false,
Expand All @@ -533,7 +533,6 @@ export const ModelsList: FC<Props> = ({
},
},
{
width: '300px',
name: i18n.translate('xpack.ml.trainedModels.modelsList.modelDescriptionHeader', {
defaultMessage: 'Description',
}),
Expand Down Expand Up @@ -567,6 +566,7 @@ export const ModelsList: FC<Props> = ({
},
},
{
width: '15%',
field: ModelsTableToConfigMapping.type,
name: i18n.translate('xpack.ml.trainedModels.modelsList.typeHeader', {
defaultMessage: 'Type',
Expand All @@ -586,9 +586,9 @@ export const ModelsList: FC<Props> = ({
</EuiFlexGroup>
),
'data-test-subj': 'mlModelsTableColumnType',
width: '130px',
},
{
width: '10%',
field: 'state',
name: i18n.translate('xpack.ml.trainedModels.modelsList.stateHeader', {
defaultMessage: 'State',
Expand All @@ -604,9 +604,9 @@ export const ModelsList: FC<Props> = ({
) : null;
},
'data-test-subj': 'mlModelsTableColumnDeploymentState',
width: '130px',
},
{
width: '20%',
field: ModelsTableToConfigMapping.createdAt,
name: i18n.translate('xpack.ml.trainedModels.modelsList.createdAtHeader', {
defaultMessage: 'Created at',
Expand All @@ -615,10 +615,9 @@ export const ModelsList: FC<Props> = ({
render: (v: number) => dateFormatter(v),
sortable: true,
'data-test-subj': 'mlModelsTableColumnCreatedAt',
width: '210px',
},
{
width: '150px',
width: '15%',
name: i18n.translate('xpack.ml.trainedModels.modelsList.actionsHeader', {
defaultMessage: 'Actions',
}),
Expand Down Expand Up @@ -768,7 +767,7 @@ export const ModelsList: FC<Props> = ({
<EuiSpacer size="m" />
<div data-test-subj="mlModelsTableContainer">
<EuiInMemoryTable<ModelItem>
css={{ overflowX: 'auto' }}
responsiveBreakpoint={'xl'}
Copy link
Member

Choose a reason for hiding this comment

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

The downloads text overflows when the width is reduced to below 1200px.

image

I think this xl limit is too high as 1200px isn't that small. The table is still readable at that width.
Setting this to l rather than xl would be better IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jgowdyelastic

  • Download action has been fixed in 9436c2f

  • This is how the table looks with the l breakpoint. The state column does not look good

image

Copy link
Member

Choose a reason for hiding this comment

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

Ok cool. Yeah that overflow was the biggest issue.
I still think 1200px is a bit high of a limit to be switching over to the other view, but I don't feel that strongly about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also don't have a strong opinion on this, let's ask @qn895 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Testing this, I'm leaning towards sticking with xl.

Copy link
Member

Choose a reason for hiding this comment

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

I think the screenshot as is looks better for 1200px 👍 If we want to go extra we can do some logic to only show partial info (like maybe just the colored dots) but that's also hiding important information. So for now this LGTM.

allowNeutralSort={false}
columns={columns}
itemIdToExpandedRowMap={itemIdToExpandedRowMap}
Expand Down