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

refactor: decouple tooltip from XY chart #553

Merged
merged 18 commits into from
Feb 20, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
12 changes: 4 additions & 8 deletions src/components/legend/_legend_item.scss
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ $legendItemVerticalPadding: $echLegendRowGap / 2;
width: 100%;

&:hover {
.echLegendItem__title {
.echLegendItem__label {
text-decoration: underline;
}
}
Expand All @@ -27,7 +27,7 @@ $legendItemVerticalPadding: $echLegendRowGap / 2;
}
}

&__title {
&__label {
@include euiFontSizeXS;
@include euiTextTruncate;
flex: 1 1 auto;
Expand All @@ -37,17 +37,13 @@ $legendItemVerticalPadding: $echLegendRowGap / 2;
}
}

&__title--selected {
text-decoration: underline;
}

&__title--hasClickListener {
&__label--hasClickListener {
&:hover {
cursor: pointer;
}
}

&__displayValue {
&__extra {
@include euiFontSizeXS;
text-align: right;
margin-left: $euiSizeXS;
Expand Down
4 changes: 2 additions & 2 deletions src/components/legend/legend.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ describe('Legend', () => {
it('shall render the all the series names without the data value', () => {
const wrapper = mount(
<Chart>
<Settings showLegend showLegendDisplayValue={false} />
<Settings showLegend showLegendExtra={false} />
<BarSeries
id="areas"
name="area"
Expand Down Expand Up @@ -127,7 +127,7 @@ describe('Legend', () => {
expect(legendItems).toHaveLength(4);
legendItems.forEach((legendItem, i) => {
// the click is only enabled on the title
legendItem.find('.echLegendItem__title').simulate('click');
legendItem.find('.echLegendItem__label').simulate('click');
expect(onLegendItemClick).toBeCalledTimes(i + 1);
});
});
Expand Down
11 changes: 5 additions & 6 deletions src/components/legend/legend.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -138,19 +138,18 @@ class LegendComponent extends React.Component<LegendProps> {
}
const { key, displayValue, banded } = item;
const { legendItemTooltipValues, settings } = this.props;
const { showLegendDisplayValue, legendPosition } = settings;
const { showLegendExtra, legendPosition } = settings;
const legendValues = this.getLegendValues(legendItemTooltipValues, key, banded);
return legendValues.map((value, index) => {
const yAccessor: BandedAccessorType = index === 0 ? BandedAccessorType.Y1 : BandedAccessorType.Y0;
return (
<LegendListItem
{...item}
label={getItemLabel(item, yAccessor)}
key={`${key}-${yAccessor}`}
legendItem={item}
displayValue={value !== '' ? value : displayValue.formatted[yAccessor]}
showLegendDisplayValue={showLegendDisplayValue}
legendPosition={legendPosition}
legendItem={item}
label={getItemLabel(item, yAccessor)}
extra={value !== '' ? value : displayValue.formatted[yAccessor]}
showExtra={showLegendExtra}
toggleDeselectSeriesAction={this.props.onToggleDeselectSeriesAction}
legendItemOutAction={this.props.onLegendItemOutAction}
legendItemOverAction={this.props.onLegendItemOverAction}
Expand Down
84 changes: 42 additions & 42 deletions src/components/legend/legend_item.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,11 @@ import { Position } from '../../utils/commons';
import { XYChartSeriesIdentifier } from '../../chart_types/xy_chart/utils/series';

interface LegendItemProps {
selectedLegendItem?: LegendItem | null;
legendItem: LegendItem;
displayValue: string;
extra: string;
label?: string;
legendPosition: Position;
showLegendDisplayValue: boolean;
showExtra: boolean;
onLegendItemClickListener?: LegendItemListener;
onLegendItemOutListener?: BasicListener;
onLegendItemOverListener?: LegendItemListener;
Expand All @@ -24,45 +23,41 @@ interface LegendItemProps {
}

/**
* Create a div for the the displayed value
* @param displayValue
* Create a div for the extra text
* @param extra
* @param isSeriesVisible
*/
function renderDisplayValue(displayValue: string, isSeriesVisible: boolean | undefined) {
const displayValueClassNames = classNames('echLegendItem__displayValue', {
['echLegendItem__displayValue--hidden']: !isSeriesVisible,
function renderExtra(extra: string, isSeriesVisible: boolean | undefined) {
const extraClassNames = classNames('echLegendItem__extra', {
['echLegendItem__extra--hidden']: !isSeriesVisible,
});
return (
<div className={displayValueClassNames} title={displayValue}>
{displayValue}
<div className={extraClassNames} title={extra}>
{extra}
</div>
);
}

/**
* Create a div for the title
* @param title
* @param onTitleClick
* @param hasTitleClickListener
* @param isSelected
* @param showLegendDisplayValue
* Create a div for the label
* @param label
* @param onLabelClick
* @param hasLabelClickListener
*/
function renderTitle(
title: string,
onTitleClick: (event: React.MouseEvent<Element, MouseEvent>) => void,
hasTitleClickListener: boolean,
isSelected: boolean,
showLegendDisplayValue: boolean,
function renderLabel(
onLabelClick: (event: React.MouseEvent<Element, MouseEvent>) => void,
hasLabelClickListener: boolean,
label?: string,
) {
// TODO add contextual menu panel on click
const titleClassNames = classNames('echLegendItem__title', {
['echLegendItem__title--hasClickListener']: hasTitleClickListener,
['echLegendItem__title--selected']: isSelected,
['echLegendItem__title--hasDisplayValue']: showLegendDisplayValue,
if (!label) {
return null;
}
const labelClassNames = classNames('echLegendItem__label', {
['echLegendItem__label--hasClickListener']: hasLabelClickListener,
});
return (
<div className={titleClassNames} title={title} onClick={onTitleClick}>
{title}
<div className={labelClassNames} title={label} onClick={onLabelClick}>
{label}
</div>
);
}
Expand All @@ -72,7 +67,10 @@ function renderTitle(
* @param color
* @param isSeriesVisible
*/
function renderColor(color: string, isSeriesVisible = true) {
function renderColor(color?: string, isSeriesVisible = true) {
if (!color) {
return null;
}
// TODO add color picker
if (isSeriesVisible) {
return (
Expand All @@ -97,24 +95,26 @@ export class LegendListItem extends React.Component<LegendItemProps> {
}

render() {
const { displayValue, legendItem, legendPosition, label } = this.props;
const { extra, legendItem, legendPosition, label, showExtra, onLegendItemClickListener } = this.props;
const { color, isSeriesVisible, seriesIdentifier, isLegendItemVisible } = legendItem;
const onTitleClick = this.onVisibilityClick(seriesIdentifier);

const { showLegendDisplayValue, selectedLegendItem, onLegendItemClickListener } = this.props;
const isSelected =
selectedLegendItem == null ? false : selectedLegendItem.seriesIdentifier.key === seriesIdentifier.key;
const hasTitleClickListener = Boolean(onLegendItemClickListener);
const itemClasses = classNames('echLegendItem', `echLegendItem--${legendPosition}`, {
const onLabelClick = this.onVisibilityClick(seriesIdentifier);
const hasLabelClickListener = Boolean(onLegendItemClickListener);

const itemClassNames = classNames('echLegendItem', `echLegendItem--${legendPosition}`, {
'echLegendItem-isHidden': !isSeriesVisible,
'echLegendItem__displayValue--hidden': !isLegendItemVisible,
'echLegendItem__extra--hidden': !isLegendItemVisible,
});

return (
<div className={itemClasses} onMouseEnter={this.onLegendItemMouseOver} onMouseLeave={this.onLegendItemMouseOut}>
{color && renderColor(color, isSeriesVisible)}
{label && renderTitle(label, onTitleClick, hasTitleClickListener, isSelected, showLegendDisplayValue)}
{showLegendDisplayValue && renderDisplayValue(displayValue, isSeriesVisible)}
<div
className={itemClassNames}
onMouseEnter={this.onLegendItemMouseOver}
onMouseLeave={this.onLegendItemMouseOut}
>
{renderColor(color, isSeriesVisible)}
{renderLabel(onLabelClick, hasLabelClickListener, label)}
{showExtra && renderExtra(extra, isSeriesVisible)}
</div>
);
}
Expand Down
2 changes: 1 addition & 1 deletion src/mocks/specs/specs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ export class MockGlobalSpec {
snap: true,
},
legendPosition: Position.Right,
showLegendDisplayValue: true,
showLegendExtra: true,
hideDuplicateAxes: false,
theme: LIGHT_THEME,
};
Expand Down
6 changes: 3 additions & 3 deletions src/specs/settings.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ describe('Settings spec component', () => {
snap: false,
},
legendPosition: Position.Bottom,
showLegendDisplayValue: false,
showLegendExtra: false,
debug: true,
xDomain: { min: 0, max: 10 },
},
Expand All @@ -84,7 +84,7 @@ describe('Settings spec component', () => {
snap: false,
});
expect(settingSpec.legendPosition).toBe(Position.Bottom);
expect(settingSpec.showLegendDisplayValue).toEqual(false);
expect(settingSpec.showLegendExtra).toEqual(false);
expect(settingSpec.debug).toBe(true);
expect(settingSpec.xDomain).toEqual({ min: 0, max: 10 });
});
Expand Down Expand Up @@ -176,7 +176,7 @@ describe('Settings spec component', () => {
snap: false,
},
legendPosition: Position.Bottom,
showLegendDisplayValue: false,
showLegendExtra: false,
hideDuplicateAxes: false,
debug: true,
xDomain: { min: 0, max: 10 },
Expand Down
9 changes: 6 additions & 3 deletions src/specs/settings.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,10 @@ export interface SettingsSpec extends Spec {
tooltip: TooltipType | TooltipProps;
debug: boolean;
legendPosition: Position;
showLegendDisplayValue: boolean;
/**
* Show an extra parameter on each legend item defined by the chart type
*/
showLegendExtra: boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we rename this to hideLegendExtra so the default is false? Then it would just be.

<Settings hideLegendExtra />

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should have the extra only visible if the user explicitly enable them (#246)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok in that case shouldn’t showLegendExtra default be false?

markov00 marked this conversation as resolved.
Show resolved Hide resolved
/**
* Removes duplicate axes
*
Expand Down Expand Up @@ -123,7 +126,7 @@ export type DefaultSettingsProps =
| 'showLegend'
| 'debug'
| 'tooltip'
| 'showLegendDisplayValue'
| 'showLegendExtra'
| 'theme'
| 'legendPosition'
| 'hideDuplicateAxes';
Expand Down Expand Up @@ -155,7 +158,7 @@ export const DEFAULT_SETTINGS_SPEC: SettingsSpec = {
snap: DEFAULT_TOOLTIP_SNAP,
},
legendPosition: Position.Right,
showLegendDisplayValue: true,
showLegendExtra: true,
hideDuplicateAxes: false,
theme: LIGHT_THEME,
};
Expand Down
8 changes: 4 additions & 4 deletions src/state/selectors/get_legend_size.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,18 @@ const legendItemLabelsSelector = createCachedSelector(
[getSettingsSpecSelector, getLegendItemsSelector],
(settings, legendItems): string[] => {
const labels: string[] = [];
const { showLegendDisplayValue } = settings;
const { showLegendExtra } = settings;
legendItems.forEach((item) => {
const labelY1 = getItemLabel(item, 'y1');
if (item.displayValue.formatted.y1 !== null) {
labels.push(`${labelY1}${showLegendDisplayValue ? item.displayValue.formatted.y1 : ''}`);
labels.push(`${labelY1}${showLegendExtra ? item.displayValue.formatted.y1 : ''}`);
} else {
labels.push(labelY1);
}
if (item.banded) {
const labelY0 = getItemLabel(item, 'y0');
if (item.displayValue.formatted.y0 !== null) {
labels.push(`${labelY0}${showLegendDisplayValue ? item.displayValue.formatted.y0 : ''}`);
labels.push(`${labelY0}${showLegendExtra ? item.displayValue.formatted.y0 : ''}`);
} else {
labels.push(labelY0);
}
Expand Down Expand Up @@ -68,7 +68,7 @@ export const getLegendSizeSelector = createCachedSelector(
);

bboxCalculator.destroy();
const { showLegend, showLegendDisplayValue, legendPosition } = settings;
const { showLegend, showLegendExtra: showLegendDisplayValue, legendPosition } = settings;
const {
legend: { verticalWidth, spacingBuffer },
} = theme;
Expand Down
2 changes: 1 addition & 1 deletion stories/legend.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ export const displayValuesInLegendElements = () => {
});
return (
<Chart className={'story-chart'}>
<Settings showLegend={true} legendPosition={legendPosition} showLegendDisplayValue={showLegendDisplayValue} />
<Settings showLegend={true} legendPosition={legendPosition} showLegendExtra={showLegendDisplayValue} />
<Axis id={getAxisId('bottom')} position={Position.Bottom} title={'Bottom axis'} showOverlappingTicks={true} />
<Axis
id={getAxisId('left2')}
Expand Down