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 schema color bug #22

Closed
wants to merge 3 commits into from
Closed

Fix schema color bug #22

wants to merge 3 commits into from

Conversation

nikodraca
Copy link

@nikodraca nikodraca commented Feb 1, 2022 •
edited
Loading

The logic for inferring colors in the schema was setting any string field as a color. This is because the d3.rgb still returns an object even if the color isn't valid:

> const {rgb} = require('d3');
> rgb("not-a-color")
Rgb { r: NaN, g: NaN, b: NaN, opacity: NaN }

This checks to see if the return values are non-null.

Closes #21

@Wattenberger
Copy link
Contributor

thanks for the PR @nikodraca! this should have been handled in commit 00df00c

I think destructuring color would lead to errors, since rgb() can return null, eg for an empty string:

image

testing in this Observable notebook

definitely let me know if the current check return !!color && !Number.isNaN(color.r) is missing any use cases, and thanks for taking the time here!

@nikodraca
Copy link
Author

Ah my bad! I didn't see this was updated in the mean time.

@nikodraca nikodraca deleted the nikodraca/fix-color-bug branch February 2, 2022 15:20
@Wattenberger
Copy link
Contributor

haha I snuck it past you! thanks for the time and thought, though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Multiselect on categories
2 participants
@nikodraca @Wattenberger