-
Notifications
You must be signed in to change notification settings - Fork 2
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
Enable concatenation of charts with tiles #39
Conversation
Add a unique suffix to the tile data layer name. This ensures that a basemap can be used in a concatenated chart, whereas this used to fail with: Duplicate data set name: "tile_list".
Concatenation of charts somehow sets the height or width signal to zero in vega. In those cases, the childHeight and childWidth signals are set instead, but we cannot use these because they don't always exists and referencing an undefined signal throws an error. Instead, just allow using a manually provided width or height to allow overriding the erroneous height or width signals.
I wasn't sure how to add a test for this, given that it fixes a visual artifact |
Just running the example in #33 should give an easy way to confirm my PR fixes the root issue. |
^ Tests are failing on unrelated code (e.g. linting on |
Thank you for this PR! Let's wait a few days if @binste, original author of this repository, has time to respond. Otherwise I will try my best to take over to make sure we can get this PR merged while tests remain happy. |
Thank you @geo-mathijs for the PR and @mattijn for the ping! I can probably review later this week and cut a new release. |
Thanks again! Tested and it works great :) |
Great to hear it all works! Thanks for the quick release as well 🎊 |
I've applied two small fixes to enable concatenation of charts that include altair_tiles.
Javascript Error: Duplicate data set name: "tile_list"
width
orheight
are set to 0 in vconcat and hconcat.I also played around with a hacky fix just detecting if width or height were zero and defaulting to a big number (e.g.
windowSize()
), but I figured just allowing the width and height to be set to be much cleaner.Thanks,
M