Skip to content

[Lens] Make horizontal bar chart a first-class chart#47062

Merged
chrisdavies merged 6 commits intoelastic:masterfrom
chrisdavies:lens/horizontal-bar
Oct 2, 2019
Merged

[Lens] Make horizontal bar chart a first-class chart#47062
chrisdavies merged 6 commits intoelastic:masterfrom
chrisdavies:lens/horizontal-bar

Conversation

@chrisdavies
Copy link
Contributor

This gets rid of chart rotation in the XY chart, and adds a horizontal bar chart to the mix.

image

From the user's view, horizontal bar charts are a first-class chart, even though from an implementation, it's just the XY chart. This means horizontal bars can't be mixed and matched with other XY chart types. The reason we can't mix and match them is because A) it'd b weird to have vertical line charts, and B) the entire chart rotates, so we can't just say "rotate the bars, but don't rotate the lines".

make horizontal bars a first class citizen
@chrisdavies chrisdavies added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Lens v7.5.0 labels Oct 1, 2019
@chrisdavies chrisdavies requested review from a team, flash1293 and wylieconlon October 1, 2019 18:03
@chrisdavies
Copy link
Contributor Author

Should we remove "stacked" as a distinct type, and instead make it an attribute of the split series dimension somehow?

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

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 change LGTM, except we might even be able to simplify more


// if current state is using the same data, suggest same chart with different presentational configuration

if (seriesType !== 'line' && xValue.operation.scale === 'ordinal') {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's possible that we should remove the flip suggestion entirely, what do you think? It's unclear to me when this suggestion would be useful:

Screenshot 2019-10-01 16 49 45

@wylieconlon
Copy link
Contributor

@chrisdavies It's an interesting idea to move "stacked" into the chart configuration. Biggest question is whether the chart types in the chart switcher should also include "stacked". @cchaos Worth looking into?

Screenshot 2019-10-01 16 57 28

@cchaos
Copy link
Contributor

cchaos commented Oct 1, 2019

Yes we have icons for horizontal and horizontal stacked chart types.

image

cc @AlonaNadler

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@wylieconlon
Copy link
Contributor

I think the question about "Stacked" was actually discussed before, when we first created a Lens prototype. If I remember correctly, users were confused by having a separate checkbox.

@cchaos
Copy link
Contributor

cchaos commented Oct 1, 2019

Yeah, I think just like how we currently distinguish between vertical bar and stacked vertical bar charts as completely separate types of charts, so should we consider with horizontal bars.

@flash1293
Copy link
Contributor

It's possible that we should remove the flip suggestion entirely, what do you think? It's unclear to me when this suggestion would be useful.

@AlonaNadler proposed this originally, I think the idea is to advertise the functionality. Not super important, you are right.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

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 v7.5.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants