Skip to content

Conversation

blazecus
Copy link
Contributor

@blazecus blazecus commented Aug 12, 2025

Fixes bugs that show up when inputting an accessor as a string instead of a function.

Background

The bug can be seen clearly in this codepen: https://codepen.io/heebeegb/pen/RNWLyXB?editors=0010

Swapping the getIcon accessor to a function will make it work - this pull request calls the transform function for constant attributes as well, fixing the issue.

I changed the typing of "value" to any as the transform function has arbitrary inputs. Strings, numbers, and number arrays are all passed into setConstantValue already, this pipes them into the transform call if it exists.

Change List

  • adds a transform call in setConstantValue
  • passes in context to setConstantValue for the purposes of the transform call

Fixes bugs that show up when inputting an accessor as a string instead of a function
@blazecus
Copy link
Contributor Author

@Pessimistress Could you take a look at this PR? Thanks!

Copy link
Collaborator

@felixpalmer felixpalmer left a comment

Choose a reason for hiding this comment

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

Nice, I think I've hit this before. I think it primarily affects IconLayer as having a static icon is a common pattern.

Could you add a test case toattribute.spec.ts? 'standard accessor with transform' could be used as a guide. Run tests with yarn test browser

@blazecus blazecus requested a review from felixpalmer August 21, 2025 23:55
@blazecus
Copy link
Contributor Author

@felixpalmer sorry to ping, but could you take a look again?

@felixpalmer
Copy link
Collaborator

You need to run yarn lint fix

@blazecus
Copy link
Contributor Author

blazecus commented Sep 2, 2025

You need to run yarn lint fix

My bad, fixed it @felixpalmer

@felixpalmer
Copy link
Collaborator

The tests are still failing. Use ‘yarn test browser’ to run locally and try to debug&fix the issue

@blazecus
Copy link
Contributor Author

blazecus commented Oct 4, 2025

@felixpalmer Sorry for the long delay. There was a one line fix that seems to fix a lot of the problems - but there are still 8 "not ok" tests from test browser. That said, test browser seems to fail on the master branch too at the same Scatterplot case (# Jupyter#ScatterplotLayer), so maybe it's a problem with my computer. Would running the tests on github be any different?

@coveralls
Copy link

coveralls commented Oct 5, 2025

Coverage Status

coverage: 91.143% (+0.006%) from 91.137%
when pulling 53e6c42 on blazecus:jb/transform-update
into 8e0d49e on visgl:master.

@felixpalmer felixpalmer merged commit 929c8f2 into visgl:master Oct 5, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants