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

[Lens] Chart switch icons redesign #178328

Merged
merged 12 commits into from
Mar 19, 2024
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import './chart_switch.scss';
import React, { ReactNode } from 'react';
import { EuiFlexItem, EuiIconTip, EuiBetaBadge, EuiFlexGroup } from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import { css } from '@emotion/react';

export const getDataLossWarning = (dataLoss: 'nothing' | 'layers' | 'everything' | 'columns') => {
if (dataLoss === 'nothing') {
return;
}
if (dataLoss === 'everything') {
return i18n.translate('xpack.lens.chartSwitch.dataLossEverything', {
defaultMessage: 'Changing to this visualization clears the current configuration.',
});
}
if (dataLoss === 'layers') {
return i18n.translate('xpack.lens.chartSwitch.dataLossLayersDescription', {
defaultMessage:
'Changing to this visualization modifies currently selected layer`s configuration and removes all other layers.',
});
} else
return i18n.translate('xpack.lens.chartSwitch.dataLossColumns', {
defaultMessage: `Changing to this visualization modifies the current configuration.`,
});
};

const DataLossWarning = ({ content, id }: { content: ReactNode; id: string }) => {
if (!content) return null;
mbondyra marked this conversation as resolved.
Show resolved Hide resolved
return (
<EuiFlexItem grow={false}>
<EuiIconTip
size="s"
aria-label={i18n.translate('xpack.lens.chartSwitch.dataLossLabel', {
defaultMessage: 'Warning',
})}
type="dot"
color="warning"
content={content}
iconProps={{
className: 'lnsChartSwitch__chartIcon',
'data-test-subj': `lnsChartSwitchPopoverAlert_${id}`,
}}
/>
</EuiFlexItem>
);
};

export const ExperimentalBadge = () => {
return (
<EuiFlexItem grow={false}>
<EuiBetaBadge
css={css`
vertical-align: middle;
`}
iconType="beaker"
label={i18n.translate('xpack.lens.chartSwitch.experimentalLabel', {
defaultMessage: 'Technical preview',
})}
size="s"
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to also apply a tooltip to this badge that states "Technical preview", similar to how we do with the same badge in layer settings?

CleanShot 2024-03-14 at 16 14 36

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and done!

</EuiFlexItem>
);
};

export const ChartOptionAppend = ({
dataLoss,
showExperimentalBadge,
id,
}: {
dataLoss: 'nothing' | 'layers' | 'everything' | 'columns';
showExperimentalBadge?: boolean;
id: string;
}) => (
<EuiFlexGroup
gutterSize="xs"
responsive={false}
alignItems="center"
className="lnsChartSwitch__append"
>
{showExperimentalBadge ? <ExperimentalBadge /> : null}
<DataLossWarning content={getDataLossWarning(dataLoss)} id={id} />
</EuiFlexGroup>
);
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,15 @@
display: inline-flex;
}

.lnsChartSwitch__option {
.euiSelectableListItem__text {
flex-grow: 0;
}
.euiSelectableListItem__append {
margin-left: $euiSizeXS;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

These style overrides are causing the selectable list's "enter" key badge that appears on focus to be in the wrong position. This focus badge should be aligned right, but this change causes it to appear to the left and immediately adjacent to the selectable list text content.

CleanShot 2024-03-14 at 16 22 26

Would it be possible to fix this? My first thought would be to try moving the technical preview badge and warning icon into the .euiSelectableListItem__text element, rather than using the append prop. If that doesn't work, you could also try styling a parent container of the badge and icon elements to occupy the full space between the selectable text and the focus badge. Just let me know if you want to chat 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.

Mar-15-2024 10-10-16
Fixed it with styles manipulation, we cannot pass node to the text element you mention, just a string.


// Targeting img as this won't target normal EuiIcon's only the custom svgs's
img.lnsChartSwitch__chartIcon { // stylelint-disable-line selector-no-qualifying-type
// The large icons aren't square so max out the width to fill the height
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,16 @@ import {
mockDatasourceMap,
mockDatasourceStates,
renderWithReduxStore,
} from '../../../mocks';
} from '../../../../mocks';

import {
Visualization,
FramePublicAPI,
DatasourcePublicAPI,
SuggestionRequest,
} from '../../../types';
} from '../../../../types';
import { ChartSwitch, ChartSwitchProps } from './chart_switch';
import { LensAppState, applyChanges } from '../../../state_management';
import { LensAppState, applyChanges } from '../../../../state_management';

describe('chart_switch', () => {
function generateVisualization(id: string): jest.Mocked<Visualization> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,7 @@ import {
EuiFlexGroup,
EuiFlexItem,
EuiSelectable,
EuiIconTip,
EuiSelectableOption,
EuiBadge,
} from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import { FormattedMessage } from '@kbn/i18n-react';
Expand All @@ -28,9 +26,9 @@ import {
VisualizationMap,
DatasourceMap,
Suggestion,
} from '../../../types';
import { getSuggestions, switchToSuggestion } from '../suggestion_helpers';
import { showMemoizedErrorNotification } from '../../../lens_ui_errors';
} from '../../../../types';
import { getSuggestions, switchToSuggestion } from '../../suggestion_helpers';
import { showMemoizedErrorNotification } from '../../../../lens_ui_errors';
import {
insertLayer,
removeLayers,
Expand All @@ -41,8 +39,9 @@ import {
selectActiveDatasourceId,
selectVisualization,
selectDatasourceStates,
} from '../../../state_management';
import { generateId } from '../../../id_generator/id_generator';
} from '../../../../state_management';
import { generateId } from '../../../../id_generator/id_generator';
import { ChartOptionAppend } from './chart_option_append';

interface VisualizationSelection {
visualizationId: string;
Expand Down Expand Up @@ -317,8 +316,8 @@ export const ChartSwitch = memo(function ChartSwitch({
.sort((a, b) => {
return (a.fullLabel || a.label).localeCompare(b.fullLabel || b.label);
})
.map(
(v): SelectableEntry => ({
.map((v): SelectableEntry => {
return {
'aria-label': v.fullLabel || v.label,
className: 'lnsChartSwitch__option',
isGroupLabel: false,
Expand All @@ -331,50 +330,16 @@ export const ChartSwitch = memo(function ChartSwitch({
),
append:
v.selection.dataLoss !== 'nothing' || v.showExperimentalBadge ? (
<EuiFlexGroup
gutterSize="xs"
responsive={false}
alignItems="center"
className="lnsChartSwitch__append"
>
{v.selection.dataLoss !== 'nothing' ? (
<EuiFlexItem grow={false}>
<EuiIconTip
aria-label={i18n.translate('xpack.lens.chartSwitch.dataLossLabel', {
defaultMessage: 'Warning',
})}
type="warning"
color="warning"
content={i18n.translate(
'xpack.lens.chartSwitch.dataLossDescription',
{
defaultMessage:
'Selecting this visualization type will remove incompatible configuration options and multiple layers, if present',
}
)}
iconProps={{
className: 'lnsChartSwitch__chartIcon',
'data-test-subj': `lnsChartSwitchPopoverAlert_${v.id}`,
}}
/>
</EuiFlexItem>
) : null}
{v.showExperimentalBadge ? (
<EuiFlexItem grow={false}>
<EuiBadge color="hollow">
<FormattedMessage
id="xpack.lens.chartSwitch.experimentalLabel"
defaultMessage="Technical preview"
/>
</EuiBadge>
</EuiFlexItem>
) : null}
</EuiFlexGroup>
<ChartOptionAppend
dataLoss={v.selection.dataLoss}
showExperimentalBadge={v.showExperimentalBadge}
id={v.selection.subVisualizationId}
/>
) : null,
// Apparently checked: null is not valid for TS
...(subVisualizationId === v.id && { checked: 'on' }),
})
)
};
})
);
}),
visualizationsLookup: lookup,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

export { ChartSwitch, ChartSwitchProps } from './chart_switch';
mbondyra marked this conversation as resolved.
Show resolved Hide resolved
3 changes: 1 addition & 2 deletions x-pack/plugins/translations/translations/fr-FR.json
Original file line number Diff line number Diff line change
Expand Up @@ -21482,7 +21482,6 @@
"xpack.lens.chart.labelVisibility.auto": "Auto",
"xpack.lens.chart.labelVisibility.custom": "Personnalisé",
"xpack.lens.chart.labelVisibility.none": "Aucun",
"xpack.lens.chartSwitch.dataLossDescription": "La sélection de ce type de visualisation entraîne la suppression des options de configuration incompatibles et des calques multiples, le cas échéant.",
"xpack.lens.chartSwitch.dataLossLabel": "Avertissement",
"xpack.lens.chartSwitch.experimentalLabel": "Version d'évaluation technique",
"xpack.lens.chartTitle.unsaved": "Visualisation non enregistrée",
Expand Down Expand Up @@ -42585,4 +42584,4 @@
"xpack.serverlessObservability.nav.projectSettings": "Paramètres de projet",
"xpack.serverlessObservability.nav.visualizations": "Visualisations"
}
}
}
3 changes: 1 addition & 2 deletions x-pack/plugins/translations/translations/ja-JP.json
Original file line number Diff line number Diff line change
Expand Up @@ -21496,7 +21496,6 @@
"xpack.lens.chart.labelVisibility.auto": "自動",
"xpack.lens.chart.labelVisibility.custom": "カスタム",
"xpack.lens.chart.labelVisibility.none": "なし",
"xpack.lens.chartSwitch.dataLossDescription": "ビジュアライゼーションタイプを選択すると、互換性のない構成オプションと複数のレイヤーが削除されます(存在する場合)。",
"xpack.lens.chartSwitch.dataLossLabel": "警告",
"xpack.lens.chartSwitch.experimentalLabel": "テクニカルプレビュー",
"xpack.lens.chartTitle.unsaved": "保存されていないビジュアライゼーション",
Expand Down Expand Up @@ -42577,4 +42576,4 @@
"xpack.serverlessObservability.nav.projectSettings": "プロジェクト設定",
"xpack.serverlessObservability.nav.visualizations": "ビジュアライゼーション"
}
}
}
3 changes: 1 addition & 2 deletions x-pack/plugins/translations/translations/zh-CN.json
Original file line number Diff line number Diff line change
Expand Up @@ -21589,7 +21589,6 @@
"xpack.lens.chart.labelVisibility.auto": "自动",
"xpack.lens.chart.labelVisibility.custom": "定制",
"xpack.lens.chart.labelVisibility.none": "无",
"xpack.lens.chartSwitch.dataLossDescription": "选择此可视化类型将移除不兼容的配置选项和多个图层(如果存在)",
"xpack.lens.chartSwitch.dataLossLabel": "警告",
"xpack.lens.chartSwitch.experimentalLabel": "技术预览",
"xpack.lens.chartTitle.unsaved": "未保存的可视化",
Expand Down Expand Up @@ -42557,4 +42556,4 @@
"xpack.serverlessObservability.nav.projectSettings": "项目设置",
"xpack.serverlessObservability.nav.visualizations": "可视化"
}
}
}