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: overwritting of axis by gridlines #1204

Merged
merged 3 commits into from
Jun 18, 2021
Merged

fix: overwritting of axis by gridlines #1204

merged 3 commits into from
Jun 18, 2021

Conversation

ron-debajyoti
Copy link
Contributor

Summary

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

@kibanamachine
Copy link

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
Member

jenkins test this please

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

Jenkins test this please

@ron-debajyoti
Copy link
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
Member

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
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
Member

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
Contributor Author

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
Member

jenkins test this please

@ron-debajyoti
Copy link
Contributor Author

Tests passed !

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

@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
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
Collaborator

🎉 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
Open
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