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

fix(bar): filter zero-height bars from rendering #1281

Merged
merged 5 commits into from Aug 9, 2021

Conversation

rshen91
Copy link
Contributor

@rshen91 rshen91 commented Aug 2, 2021

Summary

The rect border does not render for bar charts with values smaller than the strokeWidth (including zero y-values).

Details

  • does not render a bar if the height is 0 (f425d13)
  • Independent from the bar border size, the border should not protrude out of the current bar and renders outside the bar size. In the example below, the border around { x: 'B', y: 20, val: 1222 }, should not exist since the y value is so small (d42a3f2)
<Chart renderer="canvas">
      <Settings theme={useBaseTheme()} />
      <Axis id="count" title="count" position={Position.Left} />
      <Axis id="x" title="goods" position={Position.Bottom} />
      <BarSeries
        id="bars"
        name="amount"
        xScaleType={ScaleType.Ordinal}
        xAccessor="x"
        yAccessors={['y']}
        barSeriesStyle={{
          rectBorder: {
            visible: true,
            strokeWidth: 10,
            stroke: 'black',
          },
        }}
        data={[
          { x: 'A', y: 0, val: 1222 },
          { x: 'B', y: 20, val: 1222 },
          { x: 'C', y: 750, val: 1222 },
          { x: 'D', y: 854, val: 1222 },
        ]}
      />
    </Chart>

Issues

Fixes #1279

Checklist

  • The proper chart type label was added (e.g. :xy, :partition) if the PR involves a specific chart type
  • The proper feature label was added (e.g. :interactions, :axis) if the PR involves a specific chart feature
  • Proper documentation or storybook story was added for features that require explanation or tutorials

@rshen91 rshen91 marked this pull request as draft August 2, 2021 18:27
@rshen91 rshen91 changed the title fix(bars): remove bars with y equal to 0 fix(bar): remove bars with y equal to 0 Aug 2, 2021
@rshen91 rshen91 added :styling Styling related issue :xy Bar/Line/Area chart related labels Aug 3, 2021
@rshen91 rshen91 marked this pull request as ready for review August 3, 2021 13:16
Copy link
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

LGTM, tested locally.

Comment on lines +51 to +54
width:
themeRectBorderStyle.visible && rect.height > themeRectBorderStyle.strokeWidth
? themeRectBorderStyle.strokeWidth
: 0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a tough choice, I would think to be consistent with the border something like...

image

But then the color is completely unknown for that bar so this...

image

would be better in that case. I think this is fine just wanted to make a mention of it. In any case this is hopefully a extreme edge case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree - hopefully this is an extreme edge case.

@nickofthyme nickofthyme changed the title fix(bar): remove bars with y equal to 0 fix(bar): filter render zero height bars Aug 9, 2021
@nickofthyme nickofthyme changed the title fix(bar): filter render zero height bars fix(bar): filter zero-height bars from rendering Aug 9, 2021
@rshen91 rshen91 merged commit e324521 into elastic:master Aug 9, 2021
nickofthyme pushed a commit that referenced this pull request Aug 9, 2021
## [33.2.1](v33.2.0...v33.2.1) (2021-08-09)

### Bug Fixes

* **bar:** filter zero-height bars from rendering ([#1281](#1281)) ([e324521](e324521)), closes [#1279](#1279)
@nickofthyme
Copy link
Collaborator

🎉 This PR is included in version 33.2.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nickofthyme nickofthyme added the released Issue released publicly label Aug 9, 2021
monfera added a commit to monfera/elastic-charts that referenced this pull request Aug 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Issue released publicly :styling Styling related issue :xy Bar/Line/Area chart related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A Zero bar with rectBorder renders a small bar
2 participants