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

1073 enable date/time pattern selection through ColumnSettings #1087

Conversation

junaidzm13
Copy link
Contributor

@junaidzm13 junaidzm13 commented Dec 27, 2023

  • converges date and time column types to one unified date/time type, consistent
    with how some major programming languages handles datetime.
  • date/time pattern selection is only available for columns with "date/time" type.
  • changes DateTimePattern to be an object instead of strings to enable
    simultaneous selection of both date and time patterns.
  • also moves DateTimeColumnDescriptor to vuu-table-types package for consistency.

Copy link

netlify bot commented Dec 27, 2023

Deploy Preview for papaya-valkyrie-395400 canceled.

Name Link
🔨 Latest commit f227c9f
🔍 Latest deploy log https://app.netlify.com/sites/papaya-valkyrie-395400/deploys/6593ef8080db2e0008cd30f5

@junaidzm13 junaidzm13 force-pushed the 1073-add-date-time-formatting-options-to-column-settings branch from 0364347 to fec2dd7 Compare December 27, 2023 11:37
@junaidzm13 junaidzm13 marked this pull request as ready for review December 27, 2023 11:37
@junaidzm13 junaidzm13 changed the title 1073 add date time formatting options to column settings 1073 enable date/time pattern selection through ColumnSettings Dec 27, 2023
@junaidzm13 junaidzm13 force-pushed the 1073-add-date-time-formatting-options-to-column-settings branch from fec2dd7 to 7e954b5 Compare December 27, 2023 12:01
@junaidzm13
Copy link
Contributor Author

junaidzm13 commented Dec 27, 2023

Here's how the UI looks:

Screenshot 2023-12-27 at 8 02 08 PM

Screenshot 2023-12-27 at 8 02 43 PM

Looking good. Consider also the scenario where column has no existing configuration (just a plain old long) and we want to assign date/time here.

@junaidzm13 junaidzm13 force-pushed the 1073-add-date-time-formatting-options-to-column-settings branch 5 times, most recently from 87555cb to 4c412dc Compare December 27, 2023 16:05
function formattingSettingsByColType(props: FormattingSettingsProps) {
const { column } = props;

if (isDateTimeColumn(column)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

somewhere we ought to warn the user if they do something invalid, like assigning a date/time type to a column with a serverDatatype that is not long. This is part of the weird dissonance arising out of the fact that we're mixing server assigned data types and client side config.

Maybe we ignore for now and assume we'll fix all this when we overhaul the metadata sent by server.

const { column } = props;

if (isDateTimeColumn(column)) {
return <DateTimeFormattingSettings {...props} column={column} />;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is good but there's still a gap in functionality. We're showing the data/time config settings if the column has already been configured as a
date/time type. But the config settings panel should be where we can actually assign date/time as the type to a column of type long

I see two possibilities

  1. we populate the renderer dropdown with an alternative to the 'default long renderer', we add a 'default date/time' renderer. This would assign the date/time type to a long column.
    technically we're not actually implementing a renderer, just a formatter.

  2. We add an additional UI item, to allow toggle on a long column, so user can pick between number or date/time When thet switch to date/time, we render the new settings

wdyt ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's true. To me the second option sounds better (though the first one seems easier to implement, second might need passing down a separate action handler for handling type change).

Mixing up renderers with formatters could end up confusing devs, and it could also be harder for the users to navigate in case we have many renderers.

Copy link
Contributor

@heswell heswell left a comment

Choose a reason for hiding this comment

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

all looks good. Need to add the ability to switch a long to data/time in the settings panel.
I'd suggest adding a few date and time columns to instruments-extended in the test data package) Thats where we added stuff to test the boolean columns - I extracted these to instruments-extended rather than leave them in instruments when I realized later that the extra columns broke some existing stuff. Now that instruments-extended isd a separate table, we can add anything we want.
If you view it in Tables/SIMUL/InstrumentsExtended in showcase, you can bring up the ColumnSettings -

@junaidzm13 junaidzm13 force-pushed the 1073-add-date-time-formatting-options-to-column-settings branch 2 times, most recently from 284482d to ca4370e Compare December 28, 2023 06:19
@junaidzm13
Copy link
Contributor Author

junaidzm13 commented Dec 28, 2023

This is how the table/settings look:

Screenshot 2023-12-28 at 1 54 47 PM

  • I've also edited InputCell to use unformatted values when editing, since users would ideally want to edit the original data not the formatted one. Found the issue when I tried editing a date (long timestamp). (Separate commit)

  • Also updated the available cell renderers for boolean columns (Separate commit)

@junaidzm13 junaidzm13 force-pushed the 1073-add-date-time-formatting-options-to-column-settings branch 3 times, most recently from e5d1e19 to d72d8c6 Compare December 28, 2023 11:56
@junaidzm13 junaidzm13 force-pushed the 1073-add-date-time-formatting-options-to-column-settings branch 6 times, most recently from d59cca9 to 7e0a79c Compare January 2, 2024 04:03
@junaidzm13 junaidzm13 force-pushed the 1073-add-date-time-formatting-options-to-column-settings branch 2 times, most recently from 1f671ff to 9ff9b1f Compare January 2, 2024 10:53
- converges date and time column types to one unified date/time type, consistent
  with how some major programming languages handles datetime.
- date/time pattern selection is only available for columns with "date/time" type.
- changes DateTimePattern to be an object instead of strings to enable
  simultaneous selection of both date and time patterns.
- also moves DateTimeColumnDescriptor to vuu-table-types package for consistency.
@junaidzm13 junaidzm13 force-pushed the 1073-add-date-time-formatting-options-to-column-settings branch from 9ff9b1f to 70344a7 Compare January 2, 2024 11:06
- users can now declare the type of long columns as plain number or
  date/time.
- removed isSimpleColumnType as its no longer needed.
- added a timestamps (long) column in showcase's instruments-extended
  table.
@junaidzm13 junaidzm13 force-pushed the 1073-add-date-time-formatting-options-to-column-settings branch from 70344a7 to f227c9f Compare January 2, 2024 11:11
Copy link
Contributor

@heswell heswell left a comment

Choose a reason for hiding this comment

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

looks good, plus tested it locally

@heswell heswell merged commit 4e88476 into finos:main Jan 2, 2024
9 checks passed
@junaidzm13 junaidzm13 deleted the 1073-add-date-time-formatting-options-to-column-settings branch January 8, 2024 05:54
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