Skip to content

fix: overwritting of axis by gridlines #1204

Merged
markov00 merged 3 commits intoelastic:masterfrom
ron-debajyoti:grid-lines-fix
Jun 18, 2021
Merged

fix: overwritting of axis by gridlines #1204
markov00 merged 3 commits intoelastic:masterfrom
ron-debajyoti:grid-lines-fix

Conversation

@ron-debajyoti
Copy link
Copy Markdown
Contributor

@ron-debajyoti ron-debajyoti commented Jun 15, 2021

Summary

Fixes #1203 by rendering gridlines before axes to prevent overwriting of axes.

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jun 15, 2021

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@markov00
Copy link
Copy Markdown
Collaborator

markov00 commented Jun 16, 2021

jenkins test this please

@markov00 markov00 self-requested a review June 16, 2021 17:53
@nickofthyme
Copy link
Copy Markdown
Collaborator

nickofthyme commented Jun 16, 2021

Jenkins test this please

@ron-debajyoti
Copy link
Copy Markdown
Contributor Author

ron-debajyoti commented Jun 17, 2021 •
edited
Loading

Hey @markov00 @nickofthyme ,
Based on the jenkins tests, looks like those cases are failing where the previous version of the grid lines are present. Same errors are seen on performing the integration tests.
Any suggestions ?

@markov00
Copy link
Copy Markdown
Collaborator

markov00 commented Jun 17, 2021

Hey @markov00 @nickofthyme ,
Based on the jenkins tests, looks like those cases are failing where the previous version of the grid lines are present. Same errors are seen on performing the integration tests.
Any suggestions ?

Hey @ron-debajyoti I've seen the VRTs changes, you should update the screenshot baseline as we did last time, if you are not sure how to operate we can push these changes for you

@ron-debajyoti
Copy link
Copy Markdown
Contributor Author

ron-debajyoti commented Jun 17, 2021 •
edited
Loading

you should update the screenshot baseline as we did last time, if you are not sure how to operate we can push these changes for you

I would like to continue and learn if you could guide me a bit. So basically I need to update the image snapshots of all those failing test cases right?

@markov00
Copy link
Copy Markdown
Collaborator

markov00 commented Jun 17, 2021

Awesome, thank you!
So the easiest and only way to update the VRTs is to follow the guide here: https://github.com/elastic/elastic-charts/blob/master/wiki/testing.md#visual-regression-testing

The VRTs suite basically compiles all the storybook examples into a single small application, running on a server. The app is then accessed via Puppeteer on a headless chromium instance that screenshots and compares all the various tests.
As you will read on the guide, if you run without the -u option it will create a diff folder with the differences between the baseline screenshots and your version.
Run then the same command with the -u to update all the screenshot at once
The process can take quite some time (~10 to 15 minutes) depending if this is the first time you use the VRT server or not

@ron-debajyoti
Copy link
Copy Markdown
Contributor Author

ron-debajyoti commented Jun 17, 2021

Awesome, thank you!
So the easiest and only way to update the VRTs is to follow the guide here: https://github.com/elastic/elastic-charts/blob/master/wiki/testing.md#visual-regression-testing

Thank you @markov00 . Sorry for missing out on the testing part.
I added the latest snapshots so perhaps we should jenkins retest this.

@markov00
Copy link
Copy Markdown
Collaborator

markov00 commented Jun 17, 2021

jenkins test this please

@ron-debajyoti
Copy link
Copy Markdown
Contributor Author

ron-debajyoti commented Jun 17, 2021

Tests passed !

markov00
markov00 approved these changes Jun 17, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the changes

nickofthyme
nickofthyme approved these changes Jun 17, 2021
View reviewed changes
Copy link
Copy Markdown
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.

Changes look good!

@markov00 markov00 merged commit 38ebe2d into elastic:master Jun 18, 2021
@ron-debajyoti ron-debajyoti deleted the grid-lines-fix branch June 18, 2021 15:41
nickofthyme pushed a commit that referenced this pull request Jun 29, 2021
@semantic-release-bot
chore(release): 31.0.0 [skip ci]
597638a 
# [31.0.0](v30.2.0...v31.0.0) (2021-06-29)

### Bug Fixes

* **xy:** render gridlines behind axis  ([#1204](#1204)) ([38ebe2d](38ebe2d)), closes [#1203](#1203)
* memory leak related to re-reselect cache ([#1201](#1201)) ([02025cf](02025cf))
* **partition:** getLegendItemsExtra no longer assumes a singleton ([#1199](#1199)) ([100145b](100145b))

### Features

* **annotations:** option to render rect annotations outside chart ([#1207](#1207)) ([4eda382](4eda382))
* **heatmap:** enable brushing on categorical charts ([#1212](#1212)) ([10c3493](10c3493)), closes [#1170](#1170) [#1171](#1171)
* **xy:** add onPointerUpdate debounce and trigger options ([#1194](#1194)) ([a9a9b25](a9a9b25))

### BREAKING CHANGES

* **xy:** the `PointerOverEvent` type now extends `ProjectedValues` and drops value. This effectively replaces value with `x`, `y`, `smVerticalValue` and `smHorizontalValue`.
@nickofthyme
Copy link
Copy Markdown
Collaborator

nickofthyme commented Jun 29, 2021

🎉 This PR is included in version 31.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nickofthyme nickofthyme added the released Issue released publicly label Jun 29, 2021
@markov00 markov00 mentioned this pull request Jun 29, 2021
Merged
1 task
@nickofthyme nickofthyme mentioned this pull request Aug 19, 2021
Closed
2 tasks
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
@semantic-release-bot
chore(release): 31.0.0 [skip ci]
bae925c 
# [31.0.0](elastic/elastic-charts@v30.2.0...v31.0.0) (2021-06-29)

### Bug Fixes

* **xy:** render gridlines behind axis  ([opensearch-project#1204](elastic/elastic-charts#1204)) ([bf9ccbd](elastic/elastic-charts@bf9ccbd)), closes [#1203](elastic/elastic-charts#1203)
* memory leak related to re-reselect cache ([opensearch-project#1201](elastic/elastic-charts#1201)) ([8cb6876](elastic/elastic-charts@8cb6876))
* **partition:** getLegendItemsExtra no longer assumes a singleton ([opensearch-project#1199](elastic/elastic-charts#1199)) ([ecbcc1e](elastic/elastic-charts@ecbcc1e))

### Features

* **annotations:** option to render rect annotations outside chart ([opensearch-project#1207](elastic/elastic-charts#1207)) ([ddffc00](elastic/elastic-charts@ddffc00))
* **heatmap:** enable brushing on categorical charts ([opensearch-project#1212](elastic/elastic-charts#1212)) ([5c426b3](elastic/elastic-charts@5c426b3)), closes [opensearch-project#1170](elastic/elastic-charts#1170) [opensearch-project#1171](elastic/elastic-charts#1171)
* **xy:** add onPointerUpdate debounce and trigger options ([opensearch-project#1194](elastic/elastic-charts#1194)) ([aa068f6](elastic/elastic-charts@aa068f6))

### BREAKING CHANGES

* **xy:** the `PointerOverEvent` type now extends `ProjectedValues` and drops value. This effectively replaces value with `x`, `y`, `smVerticalValue` and `smHorizontalValue`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@markov00 markov00 markov00 approved these changes

@nickofthyme nickofthyme nickofthyme approved these changes

Assignees

No one assigned

Labels

released Issue released publicly

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

The gridline hides the horizontal axis

4 participants

@ron-debajyoti @kibanamachine @markov00 @nickofthyme