kibana
kibana copied to clipboard
elastic
[Canvas] PieVis integration to Canvas.
Summary
Completes part of https://github.com/elastic/kibana/issues/121725 and https://github.com/elastic/kibana/issues/95902.
- [X] Added views/models/elements to
CanvasforpieVis. - [X] Added
legendPosition: Position.Rightas default atpieVis/mosaicVis/treemapVis/waffleVis. - [X] Added handling of values, other then
right/left/top/bottom, passed to the legendPosition argument. Now, expressions are throughing the error in such a case.
Popup text
Other
Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)
Thanks @Kunzetsov ! There will be quite a few design/copy edits coming for the sidebar, but we're currently a bit backlogged. We'll get you this feedback ASAP.
@andreadelrio, thanks for your review. Those requested changes will lead to huge changes in the Canvas' architecture and I'll need to discuss that with @crob611, especially, if it is worth those efforts and to which version we'll add it. Thanks for your detailed review. 👍
@Kunzetsov @crob611 The changes @andreadelrio proposed keep the UI in line with the behavior of existing elements and, thus, reflect patterns long-established in Canvas. This PR, as-is, is not in a shippable state from a UI perspective and - at minimum - we should consider a subset of the proposed changes prior to merging.
In particular, this pattern of switches without labels is something we should avoid.
@Kunzetsov @crob611 The changes @andreadelrio proposed keep the UI in line with the behavior of existing elements and, thus, reflect patterns long-established in Canvas. This PR, as-is, is not in a shippable state from a UI perspective and - at minimum - we should consider a subset of the proposed changes prior to merging.
In particular, this pattern of switches without labels is something we should avoid.
@ryankeairns, yes, sure, I don't expect this PR to be merged in such a state) I understand, that we must follow your guidelines. Possibly, I haven't paid enough attention to the possible solution. From the first view, I can see, that it is touching so many parts of the Canvas for now. The main problem is in the form of the expression (it has nested expressions) and in the way, Canvas is adding the configuration. Before nested expressions were not supported and I've added a simple solution some time ago. In this case, it doesn't work and makes things more complex. I need some time to find the workaround for this way of configuring the Canvas sidebar. Later, when I'll have the whole plan of updates (from the side of the code), I'll discuss them with Corey and implement them in this PR (in some of the further versions). WDYT?
Thanks for the additional context, that seems like a practical approach to me. After you speak with Corey, please share what you both feel would be possible and we can adjust our recommendations, consider alternatives, and help determine what to push off to a future PR(s).
If I recall correctly, the sidebar is or can be a subset of what is handled by the expression itself. In the past, we would aim to expose what we felt was most useful while also considering what may be too complex. In other words, if we need to omit some things form the sidebar UI, then that is an option we can consider as well.
Re: the error message, our guidelines say not to use "Whoops!". Could the title be something like "Cannot complete expression"? "Expression failed" is ok in the title, but it is also stated in the first line.
Also, I recommend changing the button name from "See details" to "View details".
Can you provide a screenshot of the text that Ryan is referring to that needs verb + noun?
I reviewed the text for the tooltips and for the toggle buttons. In most cases, I don't feel the tooltip is necessary as it repeats the title. There isn't a lot of room next to the toggle switch other than to say "Enable toggle" or "Show donut chart" which seems redundant with the title.
A better approach to the text would be the style that Andrea proposes:
This gives us more room for the switch label so it says more than repeating the title.
@gchaps, updated the code and screenshots. Could you, please, review it and approve the PR or request some changes? Thanks a lot 😃
Punctuation Add an ending period after each tooltip.
Donut hole size tooltip: Set the inner diameter of the donut hole. Specify the percentage for the inner diameter of the donut hole.
Legend settings Legend visibility and Legend position have weird breaks. Can you put all Legend settings under one category?
Legend
Visibility Position Detail Text Max lines
Alternately, break Legend visibility and Legend position into 2 lines
Legend visibility
Legend position
Label configuration
Label configuration (make label singular) Show label settings.
Other
Since the new design allows for more words, suggest making these a little longer:
Change "Data order" to:
Order of data Use original order
Change Placement to "Slice placement"
Punctuation Add an ending period after each tooltip.
For the reasons of consistency with all the rest Canvas elements, it is required to remove points at all the tooltips.
Donut hole size tooltip: Set the inner diameter of the donut hole. Specify the percentage for the inner diameter of the donut hole.
Will do
Legend settings Legend visibility and Legend position have weird breaks. Can you put all Legend settings under one category?
Legend
Visibility Position Detail Text Max lines
No, we can’t.
Label configuration
Label configuration (make label singular) Show label settings.
Will do
Other
Since the new design allows for more words, suggest making these a little longer: Change "Data order" to: Order of data Use original order
Will do
Change Placement to "Slice placement"
Will do
Although the best solution would be to add a label or to break the Legend text into two lines, how about this text? So that there is no break in the text:
Legend visibility > Legend view Legend placement > Placement
@elastic/kibana-presentation, could you, please, review this PR? Thanks 😃
And @andreadelrio, could you, please, approve it to cancel your Request changes? Thanks a lot too 😃