DatePickerCell Implementation

[willhuang1997]

Hi,

I am a software engineer at Streamlit working with Lukas Masuch. I am attempting to add date, datetime, and time support but it looks like the current implementation doesn't fully work and there are some edge cases that aren't necessary kept.

I left comments as to why most changes are specifically there. I didn't necessarily see any tests that would be implemented but I have tested this custom cell in Streamlit, in glide-data-grid, and also had Lukas look over some of the code as a review (obviously doesn't count as a review but just saying).

I would love to work with anyone who would like to help me get this in and any requests to make this work!

William Huang
SWE at Streamlit / Snowflake

[jassmith]

Most polite PR ever

[jassmith]

Did you run prettier over these files? The formatting feels off. I should have a check for that...

[willhuang1997]

Did you run prettier over these files? The formatting feels off. I should have a check for that...

Hm... I ran:

yarn prettier --write packages/

and then committed...?

[jassmith]

No problem

[jassmith]

Everything else looks great. Will try to get test support stood up ASAP so Im not blocking you.

[jassmith]

5.2.2 now has a test framework in place for cells. Please rebase your branch onto 5.2.2 and retarget.

The base branch was changed.

[lukasmasuch]

Another more optional feature (does not need to be part of this PR): Make the min (string), max (string), and step (number) attributes accessible through the cell props. These parameters are supported in the same way by the time, date, and datetime-local inputs, so I think this should be very trivial to support.

[willhuang1997]

Hi @jassmith ,

Do you have any recommendations as to how to render the editor and then somehow call the onChange method? I tried looking at cells.test.tsx and copying this code but it didn't end up working properly on my VSCode with error along the lines that Editor is not a React Component:

        const Editor = renderer.provideEditor?.(MOCK_DATE_CELL)
        const target = getMockEditorTarget();
        assert(Editor !== undefined)
        assert(!isObjectEditorCallbackResult(Editor));

        assert(Editor !== undefined);
        const result = render(
            <Editor
                onChange={noop}
                onFinishedEditing={noop}
                isHighlighted={false}
                value={MOCK_DATE_CELL}
                target={target}
                forceEditMode={false}
            />
        );

However, would this be able to get merged otherwise? I do have some TODOs that I do plan to get to but the implementation itself might be good to go after meeting with Lukas?

cc: @lukasmasuch just to sign off too

[jassmith]

I probably wont have time for about 7 days to debug what is going on there. If you can get it working I will have time to review however.

[willhuang1997]

Hi @jassmith ! I know you're busy but will you have time to review it anytime soon? Is it possible to get a date as to when this will be reviewed?

Thanks,

William

[jassmith]

I will review it tonight after dinner

[jassmith]

Looks good overall. I am curious about the 1970 choice.

[jassmith]

Why not interpret it as "today" instead of 1970? Today at least feels more likely to be expected.

[lukasmasuch]

I assume we could also use today's date here... both are probably semi-optimal solutions here. The general problem here is that there isn't really a native format for time in javascript.

[lukasmasuch]

thinking a bit more here. Probably today has slightly more advantages 👍 @willhuang1997 would you be fine as well with changing that?

[lukasmasuch]

One argument for keeping 1970-01-01: It is consistent with how the time input converts the time back into a date. Calling valueAsDate on the time input event will return the time based on 1970-01-01. valueAsNumber is also the UNIX timestamp from 1970-01-01 and not today.

[willhuang1997]

Looks good overall. I am curious about the 1970 choice.

Thanks! I think any day can be used for the sort to properly work but 1970 is used just because it's the Unix epoch. Is it possible to merge it? Only those with write access can merge :)

[lukasmasuch]

Oh, I think the tests are not yet running as part of the CI 😬 Let me quickly add that

Update: Done ✅

[jassmith]

Thank you both for your excellent work. I got this one from here. This looks great.

[willhuang1997]

Since the build was broken, I removed the failing tests because they were based off of local time zone and causing flakiness.

[lukasmasuch]

It would be great if we could make the DatePickerCell to also adapt to the content when it's automatically sized. We would need to add this to the renderer:

  measure: (ctx, cell) => {
    const { displayDate } = cell.data
    return ctx.measureText(displayDate).width + 16
  },

But in order to have that work, we need to remove this check (see the issue here):

glide-data-grid/packages/core/src/data-editor/use-column-sizer.ts

Line 24 in c9e1f3d

if (cell.kind === GridCellKind.Custom) return defaultSize;

[lukasmasuch]

I added a couple of small refinements and improvements to the PR:

• Add support for null as date value.
• Use defaultValue instead of value. Using value can lead to some strange issues.
• Check if valueAsNumber is NaN instead of empty value to determine that the created value by the user is not valid since we are using valueAsNumber and not value.
• Add measure to allow auto-resizing of the cell.
• Allow providing a timezoneOffset. This allows to let the user to edit a datetime object in the date & time of the timezone, but still get back the actual UTC date adjusted by the timezone.
• Adjust paste tests to only use strings, since pasting will always use strings anyways.

[msdrigg]

Is anything blocking this? This feature set looks great!