Skip to content

feat: Export Internationalized Date Types for DatePicker#2170

Merged
dgodinez-dh merged 2 commits intodeephaven:mainfrom
dgodinez-dh:dag_ExportDateTime
Jul 24, 2024
Merged

feat: Export Internationalized Date Types for DatePicker#2170
dgodinez-dh merged 2 commits intodeephaven:mainfrom
dgodinez-dh:dag_ExportDateTime

Conversation

@dgodinez-dh
Copy link
Copy Markdown
Contributor

@dgodinez-dh dgodinez-dh commented Jul 24, 2024

No description provided.

@dgodinez-dh dgodinez-dh requested a review from bmingles July 24, 2024 15:48
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jul 24, 2024 •
edited
Loading

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 24, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 46.67%. Comparing base (a5cb947) to head (b967e8a).

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #2170   +/-   ##
=======================================
  Coverage   46.67%   46.67%           
=======================================
  Files         692      692           
  Lines       38628    38628           
  Branches     9631     9631           
=======================================
  Hits        18028    18028           
  Misses      20589    20589           
  Partials       11       11
Flag Coverage Δ
unit 46.67% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bmingles
bmingles reviewed Jul 24, 2024
View reviewed changes
Comment thread packages/components/src/spectrum/dateAndTime.ts

export type DateValue = CalendarDate | CalendarDateTime | ZonedDateTime;

export type MappedDateValue<T> = T extends ZonedDateTime
Copy link
Copy Markdown
Contributor

@bmingles bmingles Jul 24, 2024

Choose a reason for hiding this comment

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

I would probably add a comment on these types describing why we need them.

Copy link
Copy Markdown
Contributor Author

@dgodinez-dh dgodinez-dh Jul 24, 2024

Choose a reason for hiding this comment

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

Pushed an update with some comments.

bmingles
bmingles requested changes Jul 24, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@bmingles bmingles 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. I'd suggest commenting these re-exports since they are a little more indirect than most of the Spectrum ones.

@dgodinez-dh
Copy link
Copy Markdown
Contributor Author

dgodinez-dh commented Jul 24, 2024

I have read the CLA Document and I hereby sign the CLA

deephaven-internal added a commit to deephaven/cla that referenced this pull request Jul 24, 2024
@deephaven-internal
@dgodinez-dh has signed the CLA in deephaven/web-client-ui#2170
1c631cf
bmingles
bmingles approved these changes Jul 24, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@bmingles bmingles left a comment

Choose a reason for hiding this comment

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

LGTM

@dgodinez-dh dgodinez-dh merged commit 7fb4f64 into deephaven:main Jul 24, 2024
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 24, 2024
@dgodinez-dh dgodinez-dh deleted the dag_ExportDateTime branch September 23, 2024 15:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@bmingles bmingles bmingles approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dgodinez-dh @bmingles
⚡