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.

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

Formatting for float and integer columns via Intl.NumberFormat #2563

Merged
merged 5 commits into from
Mar 12, 2024

Conversation

texodus
Copy link
Member

@texodus texodus commented Mar 11, 2024

Adds formatting options for integer and float columns. From #2539 which this PR supersedes:

This PR aims to create a universal number string formatter using the Intl.NumberFormat() constructor as the base datatype. This PR creates a column settings control on the viewer and integrates it into the Datagrid. Further work will integrate this control into the D3FC plugins and the OpenLayers plugin.

Screenshot 2024-03-11 at 11 53 11 PM

Things I changed from the predecessor PRs:

  • Redesign of the enable/disable paradigm for settings, switch to default/non-default.
  • Graphical redesign.
  • Converted radio lists to <select>
  • Fixed examples which still use columns.
  • Renamed column_config to columns_config for consistency (this is the corollary for the columns property and plugin/plugin_config).
  • Removed update_and_render(). This method is confusingly named - it does not call update(), it updates the VIew config (which is very expensive) and re-renders. The View config diffs and drops spurious updates, but it is still not behavior we should rely on, plus there is a better option in Renderer::update().
  • Refactoring/bugfixes/etc.

There are a bunch of issues remaining with this PR that I feel need to be resolved:

  • columns_config (was column_config from columns) is still a weird term. The suffix config is autological and fails to describe what distinguishes this field from its sibling columns.
  • Moving this property from plugin_config is not obviously better to my eye either. At least as a member of plugin_config, this kept the save()/restore() API for plugins simple. Additionally, this has been moved to share with e.g. group_by, which is another config depth we should conflate less (as some of these properties to to the View() constructor, but some must be removed).
  • Number formatting options still have some unexpected behavior. I like the pattern of having the UI update to resolve conflicts, but there are a lot of cases to consider.

IMO we should be unwinding this ambiguity completely, not shuffling it around. This:

{
    version: "2.7.1",
    plugin: "Datagrid",
    plugin_config: {
        columns: {
            datetime: {
                format: "custom",
                timeZone: "America/Curacao",
                datetime_color_mode: "foreground",
            },
            "Order ID": {
                format: "link",
                string_color_mode: "foreground",
                color: "#ff0000",
            },
        },
        editable: true,
        scroll_lock: false,
    },
    columns: ["Order ID", "datetime"],
    expressions: { datetime: "datetime(100)" },
}

... is now this ...:

{
    version: "2.7.1",
    plugin: "Datagrid",
    columns_config: {
        datetime: {
            format: "custom",
            timeZone: "America/Curacao",
            datetime_color_mode: "foreground",
        },
        "Order ID": {
            format: "link",
            string_color_mode: "foreground",
            color: "#ff0000",
        },
    },
    plugin_config: {
        editable: true,
        scroll_lock: false,
    },
    columns: ["Order ID", "datetime"],
    expressions: { datetime: "datetime(100)" },
}

... but IMO this makes more sense, and mirrors how this data structure is ultimately dissected during restore():

{
    version: "2.7.1",
    plugin: "Datagrid",
    style: {
        editable: true,
        scroll_lock: false,
        columns: {
            datetime: {
                format: "custom",
                timeZone: "America/Curacao",
                datetime_color_mode: "foreground",
            },
            "Order ID": {
                format: "link",
                string_color_mode: "foreground",
                color: "#ff0000",
            },
        },
    },
    view: {
        columns: ["Order ID", "datetime"],
        expressions: { datetime: "datetime(100)" },
    }
}

Refactorings like this are costly on the Perspective community (and me), but this would be complex to do today as plugin_config is untyped and the state is managed externally, so I can accept this conceit to the structure of the code for now. It is still a fairly significant breaking change to the persistence API.

ada-x64 and others added 4 commits March 10, 2024 04:53
implement plugin.can_render_column_styles

integrate can_render_column_styles to viewer

fix date/datetime not rerendering
initial datagrid integration

shim datagrid into new format

fix datagrid tests

symbols integration

refactor api

update migrations

Co-authored-by: Davis Silverman <sinistersnare@users.noreply.github.com>
Co-authored-by: Broch Stilley <brochington@gmail.com>

tests, lint

better column_config update repr

update tests
implement intl.numberFormat constructor UI

rm fixed; add migration; fix tests
@texodus texodus force-pushed the features/number-string-formatting branch from df569ef to 4f44520 Compare March 12, 2024 04:35
@texodus texodus merged commit a9477f2 into master Mar 12, 2024
7 checks passed
@texodus texodus deleted the features/number-string-formatting branch March 12, 2024 05:13
@texodus texodus mentioned this pull request Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking enhancement Feature requests or improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants