Skip to content

Add Dash table #27

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

Merged
merged 7 commits into from
Oct 1, 2021
Merged

Add Dash table #27

merged 7 commits into from
Oct 1, 2021

Conversation

mr-m-coetzee
Copy link
Contributor

Add converted dash_table component.

Note: Changed JSON serializer's configuration to use default naming standard.

Using camel-case naming forces DataTable (and probably everything else) to only work with data with camel-case field names. I.e. {| State = "online" |} does not work.

Copy link
Contributor

@kMutagene kMutagene left a comment

Choose a reason for hiding this comment

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

LGTM overall, but this really highlights that tests are desperately needed, especially for serialization. I would suggest we use the same strategy for serialization everywhere (JsonProperty vs anonymous records) though. Also, is the DashApp still correctly serialized with this setting?

@@ -120,6 +126,13 @@ module ComponentPropTypes =
Title:string
}
static member create label value disabled title = {Label=label; Value=value; Disabled=disabled; Title=title}
static member convert this =
Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to stop using the camelCase serializer settings, we should add JsonProperty attributes (like here) to all records that will not get serialized correctly with the new settings. Another example (which is why we used camelCase serialization can be seen here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point - that would be clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to make sure we're on the same page: The problem with the camel-case field names is with "external" data, i.e. when it is being used to pass data in, e.g.

DataTable.dataTable "memory-table" [
    DataTable.Attr.data [
        {| Country = "Canada" |}
        {| Country = "USA" |}
    ]
]

Country would be camel-cased and would fail to bind correctly to the header record "Country".

@mr-m-coetzee
Copy link
Contributor Author

Also, is the DashApp still correctly serialized with this setting?

Yes - As far as I can tell. Tested using Giraffe and Suave

@mr-m-coetzee
Copy link
Contributor Author

mr-m-coetzee commented Sep 29, 2021

I would suggest we use the same strategy for serialization everywhere (JsonProperty vs anonymous records) though

I'll add it to DashTable - but what about the rest? Do we want a separate Issue + PR for adding JsonProperty where anonymous records are currently being used?

@mr-m-coetzee
Copy link
Contributor Author

If issue !29 gets the go-ahead, updating to JsonProperty can be done as part of that?

@kMutagene
Copy link
Contributor

I would suggest merging this with json properties for now, and handling #29 in a separate PR. looks like going for the DynamicObj approach is the better way than using records at all for these objects anyways (see my comment on #29)

@mr-m-coetzee
Copy link
Contributor Author

Updated with JSON attributes

@kMutagene kMutagene merged commit bd45399 into plotly:dev Oct 1, 2021
@mr-m-coetzee mr-m-coetzee deleted the DashTable branch October 1, 2021 13:24
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.

2 participants