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

Native DatePicker: min and max HTML attribute shouldn't contain time #12007

Merged
merged 4 commits into from Mar 23, 2024

Conversation

buddhaCode
Copy link
Contributor

Description

Setting the minDate()/maxDate() restrictions on a native Date input doesn't restrict the selection in the GUI. Currently, the min/max attribute on the HTML tag contains the time, even when one is just using the DatePicker (not DateTimePicker). According to the HTML docs, a date input should only have the date and ommit the time.

DatePicker::make('Native Date Picker')
  ->minDate(now()->subDays(3))
  ->maxDate(now()->addDays(3))

Produces:

<input [...] max="2024-03-26 18:14:56" min="2024-03-20 18:14:56" type="date" [...]>

But should produce:

<input [...] max="2024-03-26" min="2024-03-20" type="date" [...]>

The native date time input works fine. But according to HTML standard, the space between the date and time should be an T. But Chrome also accepts the space. And the GUI only restricts the date selection, not the time selection. But I guess thats regular chrome behavior.

The JS date and date time picker work fine as expected.

Visual changes

Before (tested with Chrome 123 on Mac/Safari 17.2.1 on Mac/Firefox 124 on Mac):

Bildschirmfoto 2024-03-23 um 19 15 05

After:

Bildschirmfoto 2024-03-23 um 19 16 08

Functional changes

  • Code style has been fixed by running the composer cs command.
  • Changes have been tested to not break existing functionality.
  • Documentation is up-to-date.

@danharrin danharrin added this to the v3 milestone Mar 23, 2024
@danharrin danharrin added the bug Something isn't working label Mar 23, 2024
@buddhaCode
Copy link
Contributor Author

Hey Dan, thanks for reviewing my PRs. What can I do, that you don't have to rewrite my PRs completely? ;)

@danharrin
Copy link
Member

There's not some general tip I can write that will improve them. I do this to most PRs. There's just some insights that most people don't have that are a little specific to people who have built so much of the framework. In this case, I didn't like the original implementation since I thought it was fragile - if the type of the max date / min date variable changed in the future away from a string then substr wouldn't really work, and if the date format changed to include extra characters then it also would break as the substr indexes would be out. I just changed it to make it so that it hopefully wouldn't break in these cases, to ease maintenance in the future.

With the other PR, there were other places that also needed the implementation. I also copied the implementation from the text column/entry instead of introducing Collection::wrap() for consistency. I didn't like that a object collection was being created in all cases even though we didn't use any of the collection methods, it felt like an inefficiency.

@danharrin danharrin closed this Mar 23, 2024
@danharrin danharrin reopened this Mar 23, 2024
@danharrin danharrin merged commit 8b779eb into filamentphp:3.x Mar 23, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants