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.

Sign up for GitHub

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

Jump to bottom

fix(theme): fix grid position check #373

Merged
merged 3 commits into from
Sep 12, 2019
Merged

fix(theme): fix grid position check #373

merged 3 commits into from
Sep 12, 2019

Conversation

nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented Sep 12, 2019 •
edited
Loading

Summary

Update grid type check and update util naming

closes #372

<Settings
  theme={{
    axes: {
      gridLineStyle: {
        horizontal: {
          stroke: 'red',
          strokeWidth: 0.5,
          opacity: 1,
          dash: [0, 0],
        },
        vertical: {
          stroke: 'blue',
          strokeWidth: 0.5,
          opacity: 1,
          dash: [4, 4],
        },
      },
    },
  }}
/>

Screen Shot 2019-09-11 at 9 32 09 PM

Checklist

  • Unit tests were updated or added to match the most common scenarios
  • Each commit follows the convention

@nickofthyme
fix(theme): fix grid position check
c140c2c 
Update grid type check and update util naming

closes elastic#372
@nickofthyme nickofthyme added bug Something isn't working :axis Axis related issue :styling Styling related issue labels Sep 12, 2019
@codecov-io
Copy link

codecov-io commented Sep 12, 2019 •
edited
Loading

Codecov Report

Merging #373 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #373      +/-   ##
==========================================
+ Coverage   98.27%   98.27%   +<.01%     
==========================================
  Files          38       38              
  Lines        2782     2784       +2     
  Branches      657      657              
==========================================
+ Hits         2734     2736       +2     
  Misses         44       44              
  Partials        4        4
Impacted Files Coverage Δ
...art_types/xy_chart/annotations/annotation_utils.ts 100% <100%> (ø) ⬆️
src/chart_types/xy_chart/store/utils.ts 96.7% <100%> (ø) ⬆️
src/chart_types/xy_chart/utils/axis_utils.ts 100% <100%> (ø) ⬆️

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 db54398...33477f7. Read the comment docs.

@nickofthyme nickofthyme requested a review from monfera September 12, 2019 15:04
monfera
monfera approved these changes Sep 12, 2019
View reviewed changes
Copy link
Contributor

@monfera monfera left a comment

Choose a reason for hiding this comment

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

LGTM, indeed it was flipped and it fixes it, merge away once it passes and ticks are filled

src/chart_types/xy_chart/annotations/annotation_utils.ts Outdated
@@ -638,10 +638,10 @@ export function isVerticalAnnotationLine(isXDomainAnnotation: boolean, isHorizon
export function getAnnotationLineTooltipXOffset(chartRotation: Rotation, axisPosition: Position): number {
let xOffset = 0;

const isHorizontalAxis = isHorizontal(axisPosition);
const isHorizontal = isHorizontalAxis(axisPosition);
Copy link
Contributor

Choose a reason for hiding this comment

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

Super minor, don't bother if this would be the only code change suggestion: as it's a single-use value, whose name is similar to the name of the function, might as well be inlined as if(isHorizontalAxis(axisPosition))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great point, I was unsure about the naming. Fixed in 33477f7

src/chart_types/xy_chart/store/utils.ts
@@ -538,7 +538,7 @@ export function getAxesSpecForSpecId(axesSpecs: Map<AxisId, AxisSpec>, groupId:
if (axisSpec.groupId !== groupId) {
continue;
}
if (isVertical(axisSpec.position)) {
if (isVerticalAxis(axisSpec.position)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

like here 😃

@nickofthyme nickofthyme merged commit af4805f into elastic:master Sep 12, 2019
@nickofthyme nickofthyme deleted the fix/grid-type-check branch September 12, 2019 17:04
markov00 pushed a commit that referenced this pull request Sep 12, 2019
@semantic-release-bot
chore(release): 12.0.1 [skip ci]
eb482be 
## [12.0.1](v12.0.0...v12.0.1) (2019-09-12)

### Bug Fixes

* **theme:** fix grid position check ([#373](#373)) ([af4805f](af4805f)), closes [#372](#372)
@markov00
Copy link
Member

🎉 This PR is included in version 12.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Sep 12, 2019
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
@semantic-release-bot
chore(release): 12.0.1 [skip ci]
efbcf42 
## [12.0.1](elastic/elastic-charts@v12.0.0...v12.0.1) (2019-09-12)

### Bug Fixes

* **theme:** fix grid position check ([opensearch-project#373](elastic/elastic-charts#373)) ([cf4464f](elastic/elastic-charts@cf4464f)), closes [opensearch-project#372](elastic/elastic-charts#372)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@monfera monfera monfera approved these changes

@markov00 markov00 Awaiting requested review from markov00

@rshen91 rshen91 Awaiting requested review from rshen91

Assignees
No one assigned
Labels
:axis Axis related issue bug Something isn't working released Issue released publicly :styling Styling related issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Grid line styles are reversed
4 participants
@nickofthyme @codecov-io @markov00 @monfera