[ESQL] Add chart switch #177790
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| /* | ||
|
There was a problem hiding this comment. @MichaelMarcialis thank you for your comments! I'll keep the responses here so we can take advantage of @markov00 proposal to keep the context in one place. First issue posted by you:
There was a problem hiding this comment. I am not sure it's a way to go. Here's how it looks right now in my Macbook 16'. The scrolling area is already extremely small for ESQL charts and making it even smaller makes it not even fit one layer configuration. I think it makes it worse for the user and I'd prefer to keep scroll as it is. Whether we decide to change it or not, I can add an issue and keep it separate from this PR.
There was a problem hiding this comment. That's a fair point. Yeah, let's split it off as a separate issue to tackle, since it will likely require more design thought and possible layout shuffling (beyond the scope of this PR). Thanks! |
||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
|
There was a problem hiding this comment.
There was a problem hiding this comment. I love the beaker idea! Regarding the other one, but shouldn't be just a little scary though? We don't offer any undo functionality and user can loose a big chunk of their configuration with clicking this button. I am happy to implement it though, it's just my thoughts. There was a problem hiding this comment. I submitted an issue here: #178232 There was a problem hiding this comment. Thank you for splitting off as a separate issue. And I do share your concern about making a potentially destructive action less scary while we continue to operate without undo functionality. On the other hand, the There was a problem hiding this comment. I like it, let's do it! I think we can also make the warning more specific. For example.
Maybe we could use this information while thinking about this new design? I will create a small POC (outside of this PR) and link it to the issue There was a problem hiding this comment. I really like this proposal 😍 |
||
| * or more contributor license agreements. Licensed under the Elastic License | ||
|
There was a problem hiding this comment.
We still do it, we just don't do it when the visualization is completely cleared out. Do you think we should change it? It's possible, but I'd do it separately 🙏🏼 
There was a problem hiding this comment. Yeah, I think it'd be a better user experience to show that sort of "required" error on the appropriate dimensions even when the entire configuration is cleared out. I'm fine with splitting it out as a separate issue as well. |
||
| * 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
mbondyra marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| * in compliance with, at your election, the Elastic License 2.0 or the Server | ||
| * Side Public License, v 1. | ||
| */ | ||
|
|
||
| import React from 'react'; | ||
| import { EuiIcon, EuiText, EuiFlexGroup, EuiFlexItem, IconType } from '@elastic/eui'; | ||
| import { ToolbarButton } from '@kbn/shared-ux-button-toolbar'; | ||
| import { euiThemeVars } from '@kbn/ui-theme'; | ||
| import { css } from '@emotion/react'; | ||
|
|
||
| export const ChartSwitchTrigger = function ({ | ||
| label, | ||
| icon, | ||
| onClick, | ||
| dataTestSubj, | ||
| size = 's', | ||
| }: { | ||
| label: string; | ||
| icon?: IconType; | ||
| onClick: () => void; | ||
| dataTestSubj?: string; | ||
| size?: 's' | 'm'; | ||
| }) { | ||
| return ( | ||
| <ToolbarButton | ||
| data-test-subj={dataTestSubj} | ||
| aria-label={label} | ||
| onClick={onClick} | ||
| fullWidth | ||
| size={size} | ||
| fontWeight="bold" | ||
| label={ | ||
| size === 'm' ? ( | ||
| <ChartSwitchLabel label={label} icon={icon} /> | ||
| ) : ( | ||
| <LayerChartSwitchLabel label={label} icon={icon} /> | ||
| ) | ||
| } | ||
| /> | ||
| ); | ||
mbondyra marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| }; | ||
|
|
||
| const ChartSwitchLabel = function ({ label, icon }: { label: string; icon?: IconType }) { | ||
| return ( | ||
| <> | ||
| {icon && <EuiIcon size="l" className="lnsChartSwitch__summaryIcon" type={icon} />} | ||
| <span className="lnsChartSwitch__summaryText">{label}</span> | ||
| </> | ||
| ); | ||
| }; | ||
|
|
||
| const LayerChartSwitchLabel = function ({ label, icon }: { label: string; icon?: IconType }) { | ||
| return ( | ||
| <EuiFlexGroup gutterSize="none" alignItems="center" responsive={false}> | ||
| {icon && ( | ||
| <EuiFlexItem grow={false}> | ||
| <EuiIcon type={icon} /> | ||
| </EuiFlexItem> | ||
| )} | ||
|
|
||
| <EuiFlexItem grow={false}> | ||
| <EuiText | ||
| size="s" | ||
| css={css` | ||
| font-weight: 600; | ||
| display: inline; | ||
| padding-left: ${euiThemeVars.euiSizeS}; | ||
| `} | ||
| > | ||
| {label} | ||
| </EuiText> | ||
| </EuiFlexItem> | ||
| </EuiFlexGroup> | ||
| ); | ||
| }; | ||
mbondyra marked this conversation as resolved.
Show resolved
Hide resolved
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,9 +17,3 @@ | |
| .lnsChangeIndexPatternPopover__trigger { | ||
| padding: 0 $euiSize; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the jumpy accordion animation wasn't resolved, as I'm still seeing it in local testing. Possible to give it another look?
There are currently some issues present in the flyout when the user has greatly increased the height of the ES|QL editor and/or they have reduced the height of their browser's viewport:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you Michael! As you say, it is a can of worms – I fixed the jump before but I broke another thing and then I thought I fixed it again, but it's not fixed for all the cases and broke the second thing you mention... I will submit another issue to make sure it's tested in separation 🙏🏼