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 toolbar api #69263

Merged
merged 20 commits into from
Jun 26, 2020
Merged

[Lens] Add toolbar api #69263

merged 20 commits into from
Jun 26, 2020

Conversation

flash1293
Copy link
Contributor

@flash1293 flash1293 commented Jun 16, 2020

Fixes #67384

This PR adds a new optional property to a Lens visualization - renderToolbar. If the property is provided, it is called to render above the actual chart. This is meant to provide a place for chart-level settings.

This removes the "title" bar (title is still visible in the chrome breadcrumb)

The toolbar renderer has access to the current state, the state setter and the frame API.

Example (if toolbar is provided)
Screenshot 2020-06-16 at 15 21 27

Example (if toolbar is not provided)
Screenshot 2020-06-16 at 15 21 47

To test this, you can use the following example toolbar:

const TestToolbar = (props: any) => {
  const [open, setOpen] = useState(false);
  return (
    <EuiFlexGroup justifyContent="flexEnd">
      <EuiFlexItem grow={false}>
        <EuiPopover
          button={
            <EuiButtonEmpty
              iconType="arrowDown"
              iconSide="right"
              onClick={() => {
                setOpen(!open);
              }}
            >
              <EuiIcon type="gear" />
            </EuiButtonEmpty>
          }
          isOpen={open}
          closePopover={() => {
            setOpen(false);
          }}
        >
          <EuiFormRow label="Legend" display="rowCompressed" hasChildLabel={false}>
            <EuiSwitch
              label="Show legend"
              name="switch"
              checked={props.state ? props.state.legend.isVisible : false}
              onChange={(e) => {
                props.setState({
                  ...props.state,
                  legend: {
                    ...props.state.legend,
                    isVisible: e.target.checked,
                  },
                });
              }}
              compressed
            />
          </EuiFormRow>
        </EuiPopover>
      </EuiFlexItem>
    </EuiFlexGroup>
  );
};


// x-pack/plugins/lens/public/xy_visualization/xy_visualization.tsx

export const xyVisualization: Visualization<State, PersistableState> = {
  // ...
  renderToolbar(domElement, props) {
    render(<TestToolbar {...props} />, domElement);
  }
 // ...

This PR does not move over the chart-picker yet, this can be done in a separate PR.

@flash1293 flash1293 added Feature:Lens release_note:skip Skip the PR/issue when compiling release notes v7.9.0 v8.0.0 labels Jun 16, 2020
@flash1293 flash1293 marked this pull request as ready for review June 17, 2020 08:07
@flash1293 flash1293 requested a review from a team June 17, 2020 08:07
@flash1293 flash1293 added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Jun 17, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

I haven't yet run the code, but this doesn't look right to me because it removes the title. @cchaos you may want to look at this

@cchaos
Copy link
Contributor

cchaos commented Jun 17, 2020

Yes, let's not remove the title from the canvas area. We should still display it there similar to how panel titles are displayed in Dashboard. We should style the title as subdued when it hasn't been saved yet. (Green box below)
Image 2020-06-17 at 11 11 28 AM

Should the API take into account the moving of the chart type selection now, or do that as a follow up?

@flash1293
Copy link
Contributor Author

@cchaos Alright thanks, I will adjust the PR. Moving over the chart selector will be done in a separate PR.

@flash1293 flash1293 requested a review from a team as a code owner June 18, 2020 12:28
@flash1293
Copy link
Contributor Author

@cchaos

Unsaved

Screenshot 2020-06-18 at 14 31 42

SAved
Screenshot 2020-06-18 at 14 31 49

Not sure about the separator line below the title - it's there on master:

Unsaved
Screenshot 2020-06-18 at 14 26 11

Saved
Screenshot 2020-06-18 at 14 26 22

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

I think it's better without the border 👍

Comment on lines 68 to 77
<EuiPageContentHeader
className={classNames('lnsWorkspacePanelWrapper__pageContentHeader', {
'lnsWorkspacePanelWrapper__pageContentHeader--unsaved': !title,
})}
>
<span data-test-subj="lns_ChartTitle">
{title ||
i18n.translate('xpack.lens.chartTitle.unsaved', { defaultMessage: 'Unsaved' })}
</span>
</EuiPageContentHeader>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make sure the content header only shows when there's a chart? So don't show "Unsaved" in the empty state (alongside the graphic).

Copy link
Contributor

Choose a reason for hiding this comment

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

@flash1293 It is very important and pretty similar to it used to behave. Before, it would show the title component once you have a visualization and then saved it. Now I'm just asking if we can put a second layer of logic to show the "Unsaved" title after chart creation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cchaos I withdrew my comment, I found a nice solution :) Sorry for the noise.

@flash1293
Copy link
Contributor Author

@wylieconlon @cchaos can you check again?

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Code review LGTM since my guess is we don't actually have anything being populated in the toolbar area yet, this is just for the opportunity to.

Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

This is very close, but I found another title-related issue.

/>
);

expect(renderToolbarMock).toHaveBeenCalledWith(expect.anything(), {
Copy link
Contributor

Choose a reason for hiding this comment

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

I usually do expect.any(Element)

i18n.translate('xpack.lens.chartTitle.unsaved', { defaultMessage: 'Unsaved' })}
</span>
</EuiPageContentHeader>
)}
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic doesn't look right to me: it hides the title for a saved visualization in the following case:

  1. Go to a previously saved Lens visualization
  2. Clear the layer
  3. Title disappears?

@flash1293
Copy link
Contributor Author

@wylieconlon good point, fixed that

@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

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

Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

Changes LGTM!

@flash1293 flash1293 merged commit 67e4852 into elastic:master Jun 26, 2020
flash1293 added a commit to flash1293/kibana that referenced this pull request Jun 26, 2020
Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

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

Arghh, I've started checking too late 😅 I only had one comment about exhaustive-deps rule, but nevermind - we can do it some other time.
Functionality works good on Firefox - post-approved! 😄

}: WorkspacePanelWrapperProps) {
const setVisualizationState = useCallback(
(newState: unknown) => {
if (!activeVisualization) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add activeVisualization to dependency array of useCallback?

flash1293 added a commit that referenced this pull request Jun 26, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 26, 2020
* master: (34 commits)
  Upgrade `elliptic` dependency (`6.5.2` → `6.5.3`). (elastic#70054)
  [License Management] Do not break when `telemetry.enabled:false` (elastic#69711)
  [SECURITY] Redirect app/security to app/security/overview (elastic#70005)
  "Explore underlying data" in-chart action (elastic#69494)
  Api reference docs for state_containers and state_sync (elastic#67354)
  prep state transfer for passing embeddables by value to editor and back (elastic#69991)
  move Metrics API to start (elastic#69787)
  refactor: 💡 fix typo in embeddable (elastic#69417)
  [alerting] migrates the old `alerting` consumer to be `alerts` (elastic#69982)
  [APM]Create API to return data to be used on the Overview page (elastic#69137)
  [Lens] Fix delete button position in dimension panel for long labels (elastic#69495)
  [Lens] Add toolbar api (elastic#69263)
  Fixes bug on color picker defaults on TSVB (elastic#69889)
  [DOCS] Fixes wording in Upload a CSV section (elastic#69969)
  [Discover] Validate timerange before submitting query to ES (elastic#69363)
  [Maps] avoid using MAP_SAVED_OBJECT_TYPE constant when defining URL paths (elastic#69723)
  [Maps] Fix icon palettes are not working (elastic#69937)
  [Ingest Manager] Fix typo in constant name (elastic#69919)
  [test] skip status.allowAnonymous tests on cloud (elastic#69017)
  Fix backport (elastic#70003)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] Architectural requirement: Visualizations can provide chart-level configurator
6 participants