[Lens] Set legend pixel width #126018
[Lens] Set legend pixel width #126018
Conversation
DianaDerevyankina
added
Feature:Lens
release_note:skip
x-pack/plugins/lens/public/shared_components/legend_settings_popover.test.tsx
Outdated
Show resolved
Hide resolved
|
Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors) |
There was a problem hiding this comment.
This looks great Diana!! A small change to be consistent.
When this is disabled (legend is hidden) let's change the tooltip content to:
And a note (I don't think we can do something for it from the kibana side)
It doesn't seem to behave correctly when I have a very long text cc @markov00
| <FormattedMessage | ||
| id="xpack.lens.shared.legendSizeSetting.tooltip" | ||
| defaultMessage="Limited to max of 70% of the chart container. | ||
| Vertical legends limited to min of 30% of chart width" |
There was a problem hiding this comment.
| Vertical legends limited to min of 30% of chart width" | |
| Vertical legends limited to min of 30% of chart width." |
There was a problem hiding this comment.
Not sure it is something we can fix on the kibana side, but I think this feature does not work properly (in all chart types) when the legend moves on the top or bottom of the visualization:
Perhaps the padding direction is computed on the height rather than width in these cases? cc @markov00
Other issue I noticed is that the setting is ignored when the legend is moved inside the chart as for the XY visualizations:
Last small (nitpick) issue is the removal of the help when the field is disabled. Would it be possible to leave the help icon/tooltip?
|
@dej611 I think that when the legend is on top/bottom it affects the height and not the width. About the inside legends, good catch! We can disable it for now, when the inside position is selected. (unless we can fix it on our side) |
The problem I see in there is that when on the horizontal positioning the |
|
I agree. I am hoping we can follow this PR up with that more intentional design. I believe the primary way users will eventually do this is with draggable resizable legends. These settings could be derived based on best guess / defaults and reserved as an "advanced" option for fine tuning if we get it wrong. I 100% agree that users won't want to poke at these similar settings. this feature is a 🩹 right now and needs to go through a design process at some point (esp. once elastic-charts does the resizable legend support IMO) |
There was a problem hiding this comment.
Thanks for putting this together, @dziyanadzeraviankina. This is looking great. I've left a few questions and comments below for your review.
Additionally, seeing as these toolbar popovers continue to grow more robust and complex over time, I'm curious if now is a good time to revisit my original modus operandi of "disable form fields when the user has the ability to enable them in the current context." While that thinking works in simpler situations (where there is a desire to showcase all available features to users), I worry that it is now becoming too cluttered and visually noisy for the user to be presented with so many disabled fields that they have to stumble past.
Would it make sense to instead start hiding the fields that are currently being disabled in these toolbar popovers and only showing them when they available (as we've begun doing in the configuration flyouts, per the latest annotation designs)? Or would it be better to make a separate issue/PR for such a task?
CCing @ghudgins and @flash1293, in case they have any thoughts.
x-pack/plugins/lens/public/shared_components/legend_size_settings.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/lens/public/shared_components/legend_size_settings.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/lens/public/shared_components/legend_size_settings.tsx
Outdated
Show resolved
Hide resolved
| <EuiFieldNumber | ||
| placeholder={i18n.translate('xpack.lens.shared.legendSizeSetting.placeholder', { | ||
| defaultMessage: 'Auto', | ||
| })} | ||
| value={legendSize} | ||
| min={1} | ||
| step={1} | ||
| compressed | ||
| disabled={isDisabled} | ||
| onChange={(e) => { | ||
| const value = Number(e.target.value) || undefined; | ||
| onLegendSizeChange(value); | ||
| }} | ||
| append={i18n.translate('xpack.lens.shared.legendSizeSetting.fieldAppend', { | ||
| defaultMessage: 'px', | ||
| })} | ||
| /> |
There was a problem hiding this comment.
It feels a little weird to me that we're restricting users to a percentage range (via the min/max percentage widths/heights being applied behind the scenes), but then asking that users enter a pixel value for their desired legend width/height. I worry that the mixing of percentage and pixel dimensions could be point of confusion to users, especially given that the restricted dimension range is only indicated in the tooltip. Depending on the size of their visualization container, a single visualization may have the user's legend pixel dimensions honored in one scenario but hitting the min/max threshold in another.
Instead, would it make sense to 1) change the input type from pixels to percentages and 2) change this number input to an EuiRange slider (with accompanying showInput prop)? Doing would allow users to immediately see the min and max range via the slider, without having to consult a tooltip. It would also remove any situation of the user being confused over why their applied pixel dimensions aren't being honored when exceeding min/max thresholds, as that wouldn't be possible anymore. For example, if the legend is aligned left or right, then we would show an EuiRange slider (and accompanying showInput prop) with an available range of 30%–70%.
Also, if this is a direction we're interested in pursuing, then we can probably reconsider whether we need a tooltip for this field at all (since the range slider is a bit more self-explanatory).
Thoughts? CCing @ghudgins and @gvnmagni in case they have any thoughts on this topic.
There was a problem hiding this comment.
- change the input type from pixels to percentages
@MichaelMarcialis I don't have a strong opinion here but I can list two reasons why we should not do that:
- at a dashboard level you can't nicely align the legend of multiple charts vertically if each chart has different sizes
- the legend content doesn't resize/grow/expand in percentage as the legend. Adding configuring it in percentage means that if I configured my chart in lens (with the lens ~4/3 aspect ratio) i probably get a different aspect ratio in a dashboard. If I configured a 20% legend width in a 500px width time chart in Lens and then I'm stretching my chart to the full dashboard horizontal space (let's say 1920px), I may end up with a legend ~400px wide that is actually a bit too much.
It could be instead interesting to change, as a first iteration, the input to a selector with a bunch of predefined sizes like: small (50px), medium (100px), large (200px) xl (300px) etc so it's easier for the user to pick the same choice on multiple charts
There was a problem hiding this comment.
- change this number input to an EuiRange slider (with accompanying showInput prop)
I agree that slider could fit perfectly, but unfortunately, we can't tell the chart size from settings bar as it configures the chart without accessing its actual properties like size. That is why we have to go with that limits explanatory tooltip
There was a problem hiding this comment.
It could be instead interesting to change, as a first iteration, the input to a selector with a bunch of predefined sizes like: small (50px), medium (100px), large (200px) xl (300px) etc so it's easier for the user to pick the same choice on multiple charts
We've adopted this strategy multiple times and I think it makes sense here.
There was a problem hiding this comment.
Also, such preset-based interface will reduce a bit the current legend popover layout, which is starting to be quite articulated:
There was a problem hiding this comment.
- at a dashboard level you can't nicely align the legend of multiple charts vertically if each chart has different sizes
- the legend content doesn't resize/grow/expand in percentage as the legend. Adding configuring it in percentage means that if I configured my chart in lens (with the lens ~4/3 aspect ratio) i probably get a different aspect ratio in a dashboard. If I configured a 20% legend width in a 500px width time chart in Lens and then I'm stretching my chart to the full dashboard horizontal space (let's say 1920px), I may end up with a legend ~400px wide that is actually a bit too much.
@markov00: Excellent points. I spoke too soon and wasn't considering the repercussions this would have on dashboards.
It could be instead interesting to change, as a first iteration, the input to a selector with a bunch of predefined sizes like: small (50px), medium (100px), large (200px) xl (300px) etc so it's easier for the user to pick the same choice on multiple charts
@markov00, @dej611: I really like this suggestion for how easy it makes it for users to pick a consistent size and because it could potentially alleviate the need for us to even have an "auto" mode at all. Instead, we could simply default to whichever of these predefined sizes makes the most sense. Or do we still require an "auto" sizing mode to allow the legend to collapse down to the width/height of the legend's contents?
In any case, I think we can consolidate the auto toggle and legend size rows into a single row with either an EuiSelect or EuiButtonGroup that contains the available size options (including auto if it is still necessary). Would you be open to making such a change, @dziyanadzeraviankina?
There was a problem hiding this comment.
@MichaelMarcialis the auto legend size was a feature requested or preexisting in vislib, it has its own pros and cons but it still an opportunity. I'm totally happy to remove it (since it will alleviate our constraints and layout calculation) but I think this deserve discussing a bit more. Can we chat on Monday about that?
| <LegendSizeSettings | ||
| legendSize={legendSize} | ||
| onLegendSizeChange={onLegendSizeChange} | ||
| isDisabled={mode === 'hide'} | ||
| /> |
There was a problem hiding this comment.
My first instinct is to say that this might be better placed immediately after the "Alignment" form row. Thoughts?
There was a problem hiding this comment.
@dziyanadzeraviankina: It looks like the legend size controls moved up a bit, but not immediately after the "Alignment" row in some visualization types. Would it be possible to move above "Number of columns" in the screenshot below?
| placeholder={i18n.translate('xpack.lens.shared.legendSizeSetting.placeholder', { | ||
| defaultMessage: 'Auto', | ||
| })} |
There was a problem hiding this comment.
I've never been a big fan of the UX pattern of deleting/clearing an input field in order to revert to automatic values. I worry that such interactions are not immediately obvious to users. Would you be open to changing this so that there is a separate button group that allows users to switch between auto/custom modes prior to the dimension value input?
There was a problem hiding this comment.
@dziyanadzeraviankina: Per #126018 (comment), I think we can either do away with the "auto" toggle or merge it with the suggested predefined size controls.
| <LegendSizeSettings | ||
| legendSize={legendSize} | ||
| onLegendSizeChange={onLegendSizeChange} | ||
| isDisabled={mode === 'hide'} | ||
| /> |
There was a problem hiding this comment.
This setting doesn't appear to have any affect on legends when they are located inside a visualization. Should this setting be made unavailable in such situations?
There was a problem hiding this comment.
@dziyanadzeraviankina: I still have the option to change legend size on legends when they are located inside the visualization. Can we make the option unavailable in such situations?
stratoula
added
release_note:enhancement
backport:skip
Actually yes, if the legend is horizontal, the size is relative to its height, because it refers to the size that is taken away from the chart. In the horizontal legend the chart height is determined by how many items you have on the list and can take up to a max of two rows. If you put two charts vertically aside to each other, the legend size doesn't matter much, but when you put the horizontally aside one to each other then the legend height is important. I definitely see an option to reduce the legend width when using the horizontal position, but it serves a different objective.
I will take a look, I'm pretty sure we haven't considered that case, thanks for pointing it out |
|
@dej611 @MichaelMarcialis could you please take a look at the new interface? |
There was a problem hiding this comment.
Thanks for making those changes, @dziyanadzeraviankina. Upon re-review, I've added some additional comments, listed below.
- [Lens] Set legend pixel width #126018 (comment)
- [Lens] Set legend pixel width #126018 (comment)
- [Lens] Set legend pixel width #126018 (comment)
- [Lens] Set legend pixel width #126018 (comment)
Also, I wanted to bump my previous comment/question to @ghudgins and @flash1293 regarding the possibility of switching disabled toolbar form rows to hidden, as the number of options in these have grown quite a bit recently and are starting to look a bit messy. Any thoughts on whether we should address this now, in a separate PR, or discuss further?
I agree with your sentiment here @MichaelMarcialis but I would like to decouple it from this feature. Let's create a separate issue for this. |
|
My input @dziyanadzeraviankina :
|
|
After a quick call with @MichaelMarcialis an @flash1293 we agreed on:
|
…horizontal legends and move number of columns setting outside of legend location settings component to place legend width setting above
|
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
History
To update your PR or re-run it, just comment with: |
Closes #123803
Summary
Added control to legend settings popover that allows selecting legend width from options like
Auto,Small,Medium,LargeandExtra large. It is applicable for XY, partition, and heatmap charts with vertical legends.Moved number of columns setting outside of
LegendLocationSettingsto a separate component to place it below width setting for XY.*Setting doesn't get disabled for XY inside legends though it doesn't work for inside legends for now, but it's supposed to be fixed on the charts side.
Checklist
Delete any items that are not applicable to this PR.
For maintainers