Skip to content

Conversation

@markov00
Copy link
Member

Summary

During the redux refactoring (#281) I've forgot to compute the brush correctly on rotated charts. In this commit I also fixed the min and max value passed to the onBrushEnd to be aligned with the min/max value of the domain.

fix #527

Jan-28-2020 18-28-45

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

  • [ ] Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
  • This was checked for cross-browser compatibility, including a check against IE11
  • [ ] Proper documentation or storybook story was added for features that require explanation or tutorials
  • Unit tests were updated or added to match the most common scenarios
  • Each commit follows the convention

during the redux refactoring (elastic#281) I've forgot to compute the brush correctly on rotated charts. In
this commit I also fixed the min and max value passed to the onBrushEnd to be aligned with the
min/max value of the domain.

fix elastic#527
@markov00 markov00 added bug Something isn't working :interactions Interactions related issue labels Jan 28, 2020
Copy link
Contributor

@rshen91 rshen91 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 and tested the playground on IE 11 virtual box

@codecov-io
Copy link

codecov-io commented Jan 28, 2020

Codecov Report

Merging #528 into master will increase coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #528      +/-   ##
==========================================
+ Coverage    75.8%   75.81%   +<.01%     
==========================================
  Files         193      193              
  Lines        5808     5806       -2     
  Branches     1120     1120              
==========================================
- Hits         4403     4402       -1     
+ Misses       1388     1387       -1     
  Partials       17       17
Impacted Files Coverage Δ
...ypes/partition_chart/state/selectors/geometries.ts 75% <0%> (-1.93%) ⬇️
...ypes/partition_chart/state/selectors/scenegraph.ts 31.57% <100%> (+1.57%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c538c3c...9a18304. Read the comment docs.

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.

Nice work. Only comment I would make is to add a rotate knob to the current brush stories but doesn't have to be in this pr.

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.

Issue with -90 rotation AND 180

@markov00
Copy link
Member Author

@nickofthyme I've fixed the issues here: c3c8ff6

@markov00 markov00 requested a review from nickofthyme January 29, 2020 20:36
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.

👍

@markov00 markov00 merged commit 985ac21 into elastic:master Jan 30, 2020
@markov00 markov00 deleted the 2020-01-28_fix-rotated-brush branch January 30, 2020 15:20
markov00 pushed a commit that referenced this pull request Jan 30, 2020
# [17.0.0](v16.2.1...v17.0.0) (2020-01-30)

### Bug Fixes

* **brush:** rotate brush on rotated charts ([#528](#528)) ([985ac21](985ac21)), closes [#527](#527)

### Features

* text improvements ([#524](#524)) ([6e61700](6e61700))
* **listeners:** add seriesIdentifiers to element listeners ([#525](#525)) ([027d008](027d008)), closes [#419](#419) [#505](#505)

### BREAKING CHANGES

* **listeners:** the `onElementOver` and the `onElementClick` are now called with
`Array<[GeometryValue, SeriesIdentifier]>` instead of `Array<GeometryValue>`
* renames in `Partition` charts— `Layers`: `fillLabel.formatter`->`fillLabel.valueFormatter`; type `FillLabel`-> `FillLabelConfig`

Non-breaking changes:

* feat: the values in linked labels are rendered, just like they have been in the sectors (formerly, the value could optionally be put in the link label accessor itself)

* feat: font styling is possible separately for values: `valueFormatter` configs

* test: opacity decrease example; coloring examples

* feat: hierarchical data (`parent`, `sortIndex`) is made available to accessors (see stories, helpful with eg. coloring)

* refactor: tighter types; other code improvements
@markov00
Copy link
Member Author

🎉 This PR is included in version 17.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Jan 30, 2020
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
# [17.0.0](elastic/elastic-charts@v16.2.1...v17.0.0) (2020-01-30)

### Bug Fixes

* **brush:** rotate brush on rotated charts ([opensearch-project#528](elastic/elastic-charts#528)) ([b6c3302](elastic/elastic-charts@b6c3302)), closes [opensearch-project#527](elastic/elastic-charts#527)

### Features

* text improvements ([opensearch-project#524](elastic/elastic-charts#524)) ([f7b53c8](elastic/elastic-charts@f7b53c8))
* **listeners:** add seriesIdentifiers to element listeners ([opensearch-project#525](elastic/elastic-charts#525)) ([643ef1b](elastic/elastic-charts@643ef1b)), closes [opensearch-project#419](elastic/elastic-charts#419) [opensearch-project#505](elastic/elastic-charts#505)

### BREAKING CHANGES

* **listeners:** the `onElementOver` and the `onElementClick` are now called with
`Array<[GeometryValue, SeriesIdentifier]>` instead of `Array<GeometryValue>`
* renames in `Partition` charts— `Layers`: `fillLabel.formatter`->`fillLabel.valueFormatter`; type `FillLabel`-> `FillLabelConfig`

Non-breaking changes:

* feat: the values in linked labels are rendered, just like they have been in the sectors (formerly, the value could optionally be put in the link label accessor itself)

* feat: font styling is possible separately for values: `valueFormatter` configs

* test: opacity decrease example; coloring examples

* feat: hierarchical data (`parent`, `sortIndex`) is made available to accessors (see stories, helpful with eg. coloring)

* refactor: tighter types; other code improvements
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working :interactions Interactions related issue released Issue released publicly

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Brush is not working on rotated charts

4 participants