Skip to content

feat: date picker polishing #801

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

marcjulian
Copy link
Collaborator

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

Which package are you modifying?

Primitives

  • accordion
  • alert
  • alert-dialog
  • aspect-ratio
  • avatar
  • badge
  • breadcrumb
  • button
  • calendar
  • card
  • carousel
  • checkbox
  • collapsible
  • combobox
  • command
  • context-menu
  • data-table
  • date-picker
  • dialog
  • dropdown-menu
  • form-field
  • hover-card
  • icon
  • input
  • input-otp
  • label
  • menubar
  • navigation-menu
  • pagination
  • popover
  • progress
  • radio-group
  • scroll-area
  • select
  • separator
  • sheet
  • skeleton
  • slider
  • sonner
  • spinner
  • switch
  • table
  • tabs
  • textarea
  • toggle
  • toggle-group
  • tooltip
  • typography

Others

  • trpc
  • nx
  • repo
  • cli

What is the current behavior?

What is the new behavior?

Components

  • add buttonId input to connect a label to the button
  • removed text-muted-foreground from the placeholder text, following shadcn/ui styles
  • use chevron down icon for multi picker too and use same styles for multi picker button

Docs

Does this PR introduce a breaking change?

  • Yes
  • No

Changing the multi picker icon to chevron down to have the same styles as the normal date-picker.

Other information

@marcjulian marcjulian changed the title Feat/date picker polishing feat: date picker polishing Aug 4, 2025
@marcjulian marcjulian force-pushed the feat/date-picker-polishing branch from 5d55310 to 29c46dc Compare August 13, 2025 07:34
@MerlinMoos
Copy link
Contributor

Hey @marcjulian im not sure why we are ignoring the null value in the writeValue method, we should write also null, because the writeValue is also called, when you update a value via formcontrol. What do you think?

@marcjulian
Copy link
Collaborator Author

marcjulian commented Aug 18, 2025

@MerlinMoos do you mean this part?

/** CONROL VALUE ACCESSOR */
writeValue(value: T | null): void {
// optional FormControl is initialized with null value
if (value === null) return;
this.date.set(this.transformDate()(value));
}

The current type for the date is either T | undefined, I didn't add null to the type. But I guess we should handle the value null, rather than just skipping the write value.

public readonly date = model<T>();

Should we update the date to or should we set the date to undefined when writeValue is called with null value? What are your thoughts?

public readonly date = model<T | null>();

One more thing, should the null value be part of the transformDate function as well? (date: T | null) => T | null, too allow the user to also handle null values during transform?

@MerlinMoos
Copy link
Contributor

Hey, I made the changes in my pr for the date range picker, we just should merge your polishing and I resolved the writeValue issue and the Model as a binding, this is rasing to many change events

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