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] Add metric Viz config options, title position and sizing #124124

Merged
merged 24 commits into from
Feb 16, 2022

Conversation

shahzad31
Copy link
Contributor

@shahzad31 shahzad31 commented Jan 31, 2022

Summary

Adds options to set title position and sizing in the metric viz

i am hoping this makes sense, we need to use this User experience app.

image

Potential use cases

image

@shahzad31 shahzad31 marked this pull request as ready for review January 31, 2022 15:17
@shahzad31 shahzad31 requested review from a team as code owners January 31, 2022 15:17
@ghudgins
Copy link

related to #120386

Comment on lines 41 to 58
titleSize: {
types: ['string'],
help: i18n.translate('xpack.lens.metric.titleSize.help', {
defaultMessage: 'The chart title size.',
}),
},
titlePosition: {
types: ['string'],
help: i18n.translate('xpack.lens.metric.titlePosition.help', {
defaultMessage: 'The chart title position.',
}),
},
titleAlignPosition: {
types: ['string'],
help: i18n.translate('xpack.lens.metric.titleAlignPosition.help', {
defaultMessage: 'The chart title align position.',
}),
},
Copy link
Member

Choose a reason for hiding this comment

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

I have doubts on the namings here. title is usually referred to the title of the chart (the one that appears on the dashboard on the top/left corner of the embeddable).
In this case, this is a label on the visualization. In the Lens editor is called Display name so I think we should align the two namings.

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, the terms used for these text-based elements in Lens are contextual depending on the current visualization and where the text is used. For example, we use all of the following in difference circumstances:

  • Display name: Used as text for layer dimension items. Also used as to supply default text for XY visualization axis names and single metric visualization titles.
  • Title: Used as the primary text in for single metric visualizations.
  • Subtitle: Used as the secondary text in for single metric visualizations.
  • Axis name: Used for text adjacent to axes in in XY visualizations.
  • Label: Used for text on individual pie/donut slices and text anchored to ticks in XY visualizations.

Dashboards does use the term "panel title" for top-left text in a dashboard panel. While there could be some potential for confusion between Lens' usage of "title" and Dashboard's usage of "panel title", I don't think it's a huge concern at the moment for the following reasons:

  1. Dashboard's term is prefixed with the word "panel".
  2. I don't believe there is a current situation where these two terms would exist simultaneously (i.e. editing a panel title and editing a visualization title). However, this may change in the future depending on the direction that the seamless UX endeavor goes.

For the time being, I'm personally fine with the term "title" in this circumstance, but I will defer to others if they feel strongly about it.

Copy link
Member

Choose a reason for hiding this comment

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

@MichaelMarcialis I'm a bit confused because I don't see where in Lens you are using the words title and subtitle in the single metric visualizations.
Screenshot 2022-02-03 at 12 55 05
I see a Display name field that I can change to change the metric name, but I don't see any title or subtitle in any of our options. The only place I see the title word is in the CSS class name.
In the other metric vis (aggregated) I can see a Custom label that can be used to change the metric name.
Screenshot 2022-02-03 at 12 56 22
This is actually badly aligned with the option to show and hide that name called Show title
Screenshot 2022-02-03 at 12 56 17

The title is used on the Axis instead, where you can configure the Axis title in the top menu that changes the Display name of the vertical or horizontal axis.
Screenshot 2022-02-03 at 13 12 39

When I think of a title, to me always refers to something more descriptive like: Memory consumption in the last week and is more suitable to be places on the top-left part of the visualization (aka panel title). A label instead, is used in various places and contains usually less information, just a tag: RAM, CPU %, really similar to what name is about.

As always is a matter of consistency across the interface and I don't see that neither with the term title or the label.
cc @gvnmagni

Copy link
Contributor

@MichaelMarcialis MichaelMarcialis left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together, @shahzad31! Left you a few comments and questions for your review.

Comment on lines 41 to 58
titleSize: {
types: ['string'],
help: i18n.translate('xpack.lens.metric.titleSize.help', {
defaultMessage: 'The chart title size.',
}),
},
titlePosition: {
types: ['string'],
help: i18n.translate('xpack.lens.metric.titlePosition.help', {
defaultMessage: 'The chart title position.',
}),
},
titleAlignPosition: {
types: ['string'],
help: i18n.translate('xpack.lens.metric.titleAlignPosition.help', {
defaultMessage: 'The chart title align position.',
}),
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, the terms used for these text-based elements in Lens are contextual depending on the current visualization and where the text is used. For example, we use all of the following in difference circumstances:

  • Display name: Used as text for layer dimension items. Also used as to supply default text for XY visualization axis names and single metric visualization titles.
  • Title: Used as the primary text in for single metric visualizations.
  • Subtitle: Used as the secondary text in for single metric visualizations.
  • Axis name: Used for text adjacent to axes in in XY visualizations.
  • Label: Used for text on individual pie/donut slices and text anchored to ticks in XY visualizations.

Dashboards does use the term "panel title" for top-left text in a dashboard panel. While there could be some potential for confusion between Lens' usage of "title" and Dashboard's usage of "panel title", I don't think it's a huge concern at the moment for the following reasons:

  1. Dashboard's term is prefixed with the word "panel".
  2. I don't believe there is a current situation where these two terms would exist simultaneously (i.e. editing a panel title and editing a visualization title). However, this may change in the future depending on the direction that the seamless UX endeavor goes.

For the time being, I'm personally fine with the term "title" in this circumstance, but I will defer to others if they feel strongly about it.

@shahzad31
Copy link
Contributor Author

Thanks @markov00 and @MichaelMarcialis i have address most of your feedback

  1. Converted Align select to Button groups
  2. Remove left/right positions items
  3. Using EuiFontSize now
  4. Since size/align apply to both, have remove title prefix from them
  5. Removed EuiHorizentalRule
  6. Updated class name to better reflect
  7. Changed the title to Appearance

image

Copy link
Contributor

@MichaelMarcialis MichaelMarcialis left a comment

Choose a reason for hiding this comment

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

Thanks so much for addressing those comments, @shahzad31! After re-reviewing, I have a few additional comments.

Also, if it's cool with you, I took a quick design pass at the appearance toolbar menu (see image below). Changes include:

  • Tightened up a bit of the vertical space by consolidating the size and alignment options to a single form row.
  • Changed the title position select component to a button group.
  • Provided new prepend/append buttons to increase/decrease the text size as a faster alternative (in addition to being able to open the text size select dropdown as they can currently).

image

Any interest in making these changes as part of this PR? FYI, for the sake of space, I did end up changing the text size names back to abbreviations. Apologies for the back-and-forth on that!

@shahzad31
Copy link
Contributor Author

@MichaelMarcialis Sure, i like the update, will push the changes.

@shahzad31
Copy link
Contributor Author

@MichaelMarcialis i have updated , there is one small issue when XXL is selected, First row bit overflows, i am not sure, if we need to increase popover width.

image

Copy link
Contributor

@MichaelMarcialis MichaelMarcialis left a comment

Choose a reason for hiding this comment

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

Hey, @shahzad31. Thanks so much for making those suggested changes. Apologies for my delayed response; I'm just returning from PTO.

This looks great. Upon re-review, the majority of issues I discovered were those related to the form fields overflowing outside of the popover, which you mentioned earlier. Below is a list of suggested changes that should resolve that issue. In addition to these changes, you'll need to also change the width of the popovers via the toolbar_popover.scss styles (as it's currently too narrow to house these updated form elements). I used 440px in my quick mockups, so feel free to use that assuming it doesn't break anything.

Otherwise, I'm good to approve this, assuming those changes get made. Thanks so much again!

Comment on lines 44 to 54
<EuiButtonGroup
legend={i18n.translate('xpack.lens.metricChart.titleAlignLabel', {
defaultMessage: 'Align',
})}
options={alignButtonIcons}
idSelected={state.textAlign ?? 'center'}
onChange={(id) => {
setState({ ...state, textAlign: id as MetricState['textAlign'] });
}}
isIconOnly
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Add prop buttonSize="compressed" to match the rest of the UI.

Suggested change
<EuiButtonGroup
legend={i18n.translate('xpack.lens.metricChart.titleAlignLabel', {
defaultMessage: 'Align',
})}
options={alignButtonIcons}
idSelected={state.textAlign ?? 'center'}
onChange={(id) => {
setState({ ...state, textAlign: id as MetricState['textAlign'] });
}}
isIconOnly
/>
<EuiButtonGroup
legend={i18n.translate('xpack.lens.metricChart.titleAlignLabel', {
defaultMessage: 'Align',
})}
options={alignButtonIcons}
idSelected={state.textAlign ?? 'center'}
onChange={(id) => {
setState({ ...state, textAlign: id as MetricState['textAlign'] });
}}
isIconOnly
buttonSize="compressed"
/>

Comment on lines 35 to 44
<EuiButtonGroup
isFullWidth={true}
data-test-subj="lnsMissingValuesSelect"
legend="This is a basic group"
options={titlePositions}
idSelected={state.titlePosition ?? 'top'}
onChange={(value) => {
setState({ ...state, titlePosition: value as MetricState['titlePosition'] });
}}
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Add prop buttonSize="compressed" to match the rest of the UI.

Suggested change
<EuiButtonGroup
isFullWidth={true}
data-test-subj="lnsMissingValuesSelect"
legend="This is a basic group"
options={titlePositions}
idSelected={state.titlePosition ?? 'top'}
onChange={(value) => {
setState({ ...state, titlePosition: value as MetricState['titlePosition'] });
}}
/>
<EuiButtonGroup
isFullWidth
data-test-subj="lnsMissingValuesSelect"
legend="This is a basic group"
options={titlePositions}
idSelected={state.titlePosition ?? 'top'}
onChange={(value) => {
setState({ ...state, titlePosition: value as MetricState['titlePosition'] });
}}
buttonSize="compressed"
/>

</>
}
>
<EuiFlexGroup gutterSize="s">
Copy link
Contributor

Choose a reason for hiding this comment

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

Apply prop responsive={false} to prevent breaking at small viewport sizes.

Suggested change
<EuiFlexGroup gutterSize="s">
<EuiFlexGroup gutterSize="s" responsive={false}>

<EuiFlexItem>
<SizeOptions state={state} setState={setState} />
</EuiFlexItem>
<EuiFlexItem>
Copy link
Contributor

Choose a reason for hiding this comment

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

Prevent flex item growing with grow={false} prop.

Suggested change
<EuiFlexItem>
<EuiFlexItem grow={false}>

Comment on lines 25 to 34
<EuiFormRow
display="columnCompressed"
label={
<>
{i18n.translate('xpack.lens.metricChart.titlePositionLabel', {
defaultMessage: 'Title position',
})}
</>
}
>
Copy link
Contributor

Choose a reason for hiding this comment

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

Add fullWidth prop to match previous form row.

Suggested change
<EuiFormRow
display="columnCompressed"
label={
<>
{i18n.translate('xpack.lens.metricChart.titlePositionLabel', {
defaultMessage: 'Title position',
})}
</>
}
>
<EuiFormRow
display="columnCompressed"
fullWidth
label={
<>
{i18n.translate('xpack.lens.metricChart.titlePositionLabel', {
defaultMessage: 'Title position',
})}
</>
}
>

Comment on lines 65 to 95
<EuiSuperSelect
append={
<EuiButtonIcon
iconType="plus"
onClick={() => changeSize(1)}
isDisabled={currSizeIndex === titleSizes.length - 1}
/>
}
prepend={
<EuiButtonIcon
iconType="minus"
onClick={() => changeSize(-1)}
isDisabled={currSizeIndex === 0}
/>
}
data-test-subj="lnsMetricSizeSelect"
compressed
options={titleSizes.map((position) => {
return {
value: position.id,
dropdownDisplay: position.label,
inputDisplay: position.label,
};
})}
valueOfSelected={state.size ?? 'xl'}
onChange={(value) => {
setState({ ...state, size: value });
}}
itemLayoutAlign="top"
hasDividers
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Add fullWidth prop to ensure the component fills the available space.

Suggested change
<EuiSuperSelect
append={
<EuiButtonIcon
iconType="plus"
onClick={() => changeSize(1)}
isDisabled={currSizeIndex === titleSizes.length - 1}
/>
}
prepend={
<EuiButtonIcon
iconType="minus"
onClick={() => changeSize(-1)}
isDisabled={currSizeIndex === 0}
/>
}
data-test-subj="lnsMetricSizeSelect"
compressed
options={titleSizes.map((position) => {
return {
value: position.id,
dropdownDisplay: position.label,
inputDisplay: position.label,
};
})}
valueOfSelected={state.size ?? 'xl'}
onChange={(value) => {
setState({ ...state, size: value });
}}
itemLayoutAlign="top"
hasDividers
/>
<EuiSuperSelect
append={
<EuiButtonIcon
iconType="plus"
onClick={() => changeSize(1)}
isDisabled={currSizeIndex === titleSizes.length - 1}
/>
}
prepend={
<EuiButtonIcon
iconType="minus"
onClick={() => changeSize(-1)}
isDisabled={currSizeIndex === 0}
/>
}
data-test-subj="lnsMetricSizeSelect"
compressed
options={titleSizes.map((position) => {
return {
value: position.id,
dropdownDisplay: position.label,
inputDisplay: position.label,
};
})}
valueOfSelected={state.size ?? 'xl'}
onChange={(value) => {
setState({ ...state, size: value });
}}
itemLayoutAlign="top"
hasDividers
fullWidth
/>

@flash1293
Copy link
Contributor

@elasticmachine merge upstream

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Thank you so much for working on this @shahzad31 ! This will do wonders for Lens based dashboards.

I left two smallish issues, but otherwise it looks great to me

alignLeft: textAlign === 'left',
alignRight: textAlign === 'right',
alignCenter: textAlign === 'center',
[`titleSize${(size ?? 'xl').toUpperCase()}`]: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why the default size isn't handled on the toExpression level like position and alignment?

Copy link
Contributor

Choose a reason for hiding this comment

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

removed default here because we use default font size in class lnsMetricExpression__value and don't needed to add this class if size is not defained.

@@ -59,6 +60,9 @@ const toExpression = (
function: 'lens_metric_chart',
arguments: {
title: [attributes?.title || ''],
size: [state?.size || ''],
titlePosition: [state?.titlePosition || 'top'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Not opposed to default to "top" for new visualizations, but the old default was "bottom". So if we want to keep the default value here we have to add a migration to add in the bottom for the existing ones.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why we decide to use top as default? It can confuse users because previously we show label in the bottom in any case, is not?

@flash1293
Copy link
Contributor

Also it seems like the default font size was slightly larger on old versions:
Screenshot 2022-02-11 at 10 25 44
Screenshot 2022-02-11 at 10 25 14

Not a blocker for me but as FYI

@VladLasitsa
Copy link
Contributor

@elasticmachine merge upstream

@VladLasitsa
Copy link
Contributor

Also it seems like the default font size was slightly larger on old versions: Screenshot 2022-02-11 at 10 25 44 Screenshot 2022-02-11 at 10 25 14

Not a blocker for me but as FYI

It's because previously we used euiSize but now we are using euiFontSize. From my perspective using of euiFontSize is more correct.
image

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

LGTM, great job!

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lens 735 741 +6

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
lens 345 348 +3

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
lens 1.0MB 1.1MB +8.3KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
lens 43.0KB 43.5KB +485.0B
Unknown metric groups

API count

id before after diff
lens 396 399 +3

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@VladLasitsa VladLasitsa merged commit b7b5088 into elastic:main Feb 16, 2022
@ryankeairns
Copy link
Contributor

Thanks for this @shahzad31 ! This will really help us in improving the visual appearance of dashboards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants