Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Scatter plot: error with config value regressionLineOptions: null #578

Closed
thinkh opened this issue Oct 18, 2024 · 2 comments
Closed

Scatter plot: error with config value regressionLineOptions: null #578

thinkh opened this issue Oct 18, 2024 · 2 comments
Assignees
Labels
type: bug Something isn't working

Comments

@thinkh
Copy link
Member

thinkh commented Oct 18, 2024

Environment

  • Release number or git hash: v14.0.2
  • Browser: Chrome 130
  • Deployed / Local: local

Steps to reproduce the bug

  1. Use visualization config and run Ordino or Demo app
{
    "alphaSliderVal": 0.5,
    "color": null,
    "dragMode": "select",
    "facets": null,
    "labelColumns": [],
    "numColorScaleType": "Sequential",
    "numColumnsSelected": [],
    "regressionLineOptions": null,
    "shape": null,
    "showLabelLimit": null,
    "showLabels": "Selected",
    "showLegend": null,
    "type": "Scatter plot"
}

Observed Behavior

Image

The scatter plot does not render and points to these lines where the null check is missing:

config.regressionLineOptions.type,
config.regressionLineOptions.fitOptions,
config.regressionLineOptions.lineStyle,
config.regressionLineOptions.showStats,

I've tried to fix it like this:

    config.regressionLineOptions?.type,
    config.regressionLineOptions?.fitOptions,
    config.regressionLineOptions?.lineStyle,
    config.regressionLineOptions?.showStats,

However, that seems to be incomplete because the regression line options are incomplete in the sidebar.

Image

I'd guess that merging the default values is missing somewhere.

Expected Behavior

The scatterplot should also work correctly when passing "regressionLineOptions": null. Please also check the other optional config values. Thanks.


Thanks for taking the time to fill out this bug report 🤗
Make sure there aren't any open/closed issues for this topic 😃

@thinkh thinkh added the type: bug Something isn't working label Oct 18, 2024
@dvmoritzschoefl
Copy link
Contributor

hey @thinkh I think this is intended. null is no valid value for regressionLineOptions (it is also typed this way). This is because default props merging will only work correctly with undefined values. null will actually override the default value (with null) while undefined/optional will not. So the solution would be to just use undefined/optional for this key.

example

const obj = {};
console.log(merge(obj, { a: 'value', b: 'value' }, { a: undefined, b: null }));
// will print { a: 'value', b: null } 

@thinkh
Copy link
Member Author

thinkh commented Oct 23, 2024

Thanks for the explanation. We discussed this together with @oltionchampari and agreed that we will fix it in Ordino for now. However, we also saw that the config typings are very confusing and mixed up. I created a follow-up issue #591 to fix the wrong typings.

@thinkh thinkh closed this as completed Oct 23, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
type: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants