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: Remove x2/y2 channels for overlaying line and point #8472

Merged
merged 3 commits into from
Oct 12, 2022

Conversation

yhoonkim
Copy link
Contributor

@yhoonkim yhoonkim commented Oct 11, 2022

Fixes the overlaying line and point layers not to have x2 and y2 which is either not necessary or can cause unwanted lines.

📦 Published PR as canary version: 5.5.1--canary.8472.2a80c42.0

✨ Test out this PR locally via:

npm install vega-lite@5.5.1--canary.8472.2a80c42.0
# or 
yarn add vega-lite@5.5.1--canary.8472.2a80c42.0

@yhoonkim yhoonkim requested a review from kanitw October 11, 2022 05:09
@yhoonkim yhoonkim marked this pull request as ready for review October 11, 2022 05:10
Copy link
Member

@kanitw kanitw left a comment

Choose a reason for hiding this comment

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

Thanks. Have a minor comment.

@@ -151,7 +151,7 @@ export class PathOverlayNormalizer implements NonFacetUnitNormalizer<UnitSpecWit
...pick(markDef, ['clip', 'interpolate', 'tension', 'tooltip']),
...lineOverlay
},
encoding: overlayEncoding
encoding: omit(overlayEncoding, ['y2', 'x2'])
Copy link
Member

Choose a reason for hiding this comment

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

(minor) Probably better to do this above once

overlayEncoding = omit(overlayEncoding , ['y2', 'x2'])); 

above add comment to explain why y2/x2 shouldn't be a part of "overlayEncoding". (Describe an example that we need to trigger this case.)

@yhoonkim yhoonkim requested a review from kanitw October 11, 2022 23:36
@yhoonkim yhoonkim merged commit 43d3dee into next Oct 12, 2022
@yhoonkim yhoonkim deleted the yh/fix-area-line-with-y2 branch October 12, 2022 00:01
@vega-org-bot vega-org-bot mentioned this pull request Oct 12, 2022
6 tasks
@vega-org-bot
Copy link
Collaborator

🚀 PR was released in v5.6.0 🚀

@vega-org-bot vega-org-bot added released This PR has been released by auto shipit and removed prerelease labels Oct 14, 2022
BradyJ27 pushed a commit to BradyJ27/vega-lite that referenced this pull request Oct 19, 2023
* add example

* Remove y2/x2 channel for overlaying lines and points

* Add comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released This PR has been released by auto shipit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants