Skip to content

Conversation

@DamianPendrak
Copy link
Contributor

@DamianPendrak DamianPendrak commented Jul 10, 2025

Reverts #33603
As mentioned in #34017 :
It looks like the PR broke all deck.gl Charts, at least on the local environment. Updating the deck.gl packages break the Charts alone, but it seems like the implemented features from the PR require the packages to be updated.

I couldn't find a fix, but maybe you can @plavacquery?

Screenshot 2025-07-09 at 15 56 44

@github-actions github-actions bot added doc Namespace | Anything related to documentation plugins dependencies:npm packages labels Jul 10, 2025
@dosubot dosubot bot added the viz:charts:deck.gl Related to deck.gl charts label Jul 10, 2025
Copy link

@korbit-ai korbit-ai bot left a comment

Choose a reason for hiding this comment

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

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.
Category Issue Status
Functionality Unconditional StaticMap Rendering Without Token Check ▹ view 🧠 Incorrect
Performance Redundant String Trimming ▹ view 🧠 Incorrect
Files scanned
File Path Reviewed
superset-frontend/packages/superset-ui-core/src/validator/validateMapboxStylesUrl.ts
superset/examples/misc_dashboard.py
superset/examples/long_lat.py
superset-frontend/plugins/legacy-preset-chart-deckgl/src/DeckGLContainer.tsx
superset-frontend/plugins/legacy-preset-chart-deckgl/src/utils.ts
superset-frontend/plugins/legacy-preset-chart-deckgl/src/utilities/Shared_DeckGL.jsx
superset/views/base.py
superset/examples/deck.py
superset/config.py

Explore our documentation to understand the languages and file types we support and the files we ignore.

Check out our docs on how you can make Korbit work best for you and your team.

Loving Korbit!? Share us on LinkedIn Reddit and X

Comment on lines +29 to +30
v.trim().length > 0 &&
v.trim().startsWith('mapbox://styles/')

This comment was marked as resolved.

Comment on lines +137 to +141
<StaticMap
preserveDrawingBuffer
mapStyle={props.mapStyle || 'light'}
mapboxApiAccessToken={props.mapboxApiAccessToken}
/>

This comment was marked as resolved.

@codecov
Copy link

codecov bot commented Jul 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.98%. Comparing base (7d0fabe) to head (1d42931).

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #34123       +/-   ##
===========================================
+ Coverage        0   72.98%   +72.98%     
===========================================
  Files           0      559      +559     
  Lines           0    40516    +40516     
  Branches        0     4267     +4267     
===========================================
+ Hits            0    29571    +29571     
- Misses          0     9836     +9836     
- Partials        0     1109     +1109     
Flag Coverage Δ
hive 47.14% <100.00%> (?)
mysql 71.97% <100.00%> (?)
postgres 72.03% <100.00%> (?)
presto 50.89% <100.00%> (?)
python 72.94% <100.00%> (?)
sqlite 71.56% <100.00%> (?)
unit 100.00% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mistercrunch
Copy link
Member

As I mentioned, let's try and push forward if we can. Seems there's another issue around attribution we need to handle as well.

@plavacquery
Copy link
Contributor

Hi @DamianPendrak working on it to find out what's wong

@DamianPendrak
Copy link
Contributor Author

@plavacquery I had to revert the feature #33603 on my PR #34017 to allow for QA, and I didn't manage to bring it back before merging to master. Sorry about that! Let me know if I can help

@plavacquery
Copy link
Contributor

I have not found a solution yet. It seems that deckgl version changes have broken my feature cause of luma librairy.

Nonetheless I tried to go back to last working deckgl version and I still have the problem, have you any idea or which changes can provok that ?

@mistercrunch
Copy link
Member

Not sure what the best place to hold this conversation is, but if indeed the 2 PRs are mutually exclusive, as a maintainer I have to vouch for #33603 as it adds a great feature and bumps the deck-gl libs forward.

If this is related to colors - I have to admit I know nothing about the core issue - but I wonder if you could use tinycolor2 that's already a lib in the repo (?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies:npm doc Namespace | Anything related to documentation packages plugins size/L viz:charts:deck.gl Related to deck.gl charts

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants