Skip to content

[Form] Added options for separate date/time labels in DateTimeType. #21002

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 2, 2018
Merged

[Form] Added options for separate date/time labels in DateTimeType. #21002

merged 1 commit into from
Aug 2, 2018

Conversation

mktcode
Copy link
Contributor

@mktcode mktcode commented Dec 20, 2016

If your render date and time separately you need options for each label.

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
License MIT

Let's say you have the following form field:

$builder
    ->add('start', DateTimeType::class, [
        'date_widget' => 'single_text',
        ...
    ])
    ...

Then you can render the date and time widgets/rows/etc. separately:

<div>{{ form_row(form.start.date) }}</div>
<div>{{ form_row(form.start.time) }}</div>

But you can't provide labels for each, so what is displayed is just the uppercased field name ("Date" and "Time").

This PR adds 'date_label' and 'time_label' options, so you can do:

$builder
    ->add('start', DateTimeType::class, [
        'date_widget' => 'single_text',
        'date_label' => 'The Start Date',
        'time_label' => 'The Start Time',
        ...
    ])
    ...

@mktcode mktcode changed the title Added options for separate date/time labels. [Form] Added options for separate date/time labels in DateTimeType. Dec 20, 2016
@ogizanagi ogizanagi mentioned this pull request Dec 20, 2016
Merged
@ogizanagi
Copy link
Contributor

@mktcode : Just to mention a pending discussion on the way we'll allow to specify sub-types labels: #20887 (comment)

@xabbuh
Copy link
Member

xabbuh commented Dec 23, 2016

Would it make sense to make all options of the subtypes configurable (i.e. add options like date_options and time_options)?

@mktcode
Copy link
Contributor Author

mktcode commented Dec 24, 2016

I just handled it like the widget options (date_widget and time_widget) but on a second look making all subtype options configurable makes more sense I guess.

@ogizanagi
Copy link
Contributor

ogizanagi commented Dec 24, 2016 •
edited
Loading

Configuring every sub-types options does not really make sense to me. You shouldn't care about most of them, which are inherited or guessed within the parent type acting as encapsulation.

@nicolas-grekas nicolas-grekas added this to the 3.x milestone Dec 26, 2016
@fabpot
Copy link
Member

fabpot commented Mar 22, 2017

What's the status of this PR?

@mktcode
Copy link
Contributor Author

mktcode commented Mar 22, 2017

This PR is coherent with current handling of subtype options, so it seemed viable to me. On the other hand, in case of sub types, granting access to all of their options seems very legit too, but maybe this is a topic of its own.
So if it was my project I would merge and start a separate discussion on sup type options. But(!) as I am not that much into it maybe I don't get the whole picture.

@Shoplifter
Copy link
Contributor

+1 for accessibility reasons (http://webaim.org/techniques/forms/controls#input)
it will often make sense to visually hide the labels for time, so label_attr will also be needed to supply the appropriate class for that.

form rows:
Even on smallest screens you will want both fields to be displayed in one row (side by side)
wrapping each part of datetime in a separate div makes this difficult to handle though.

Personally I use this SO solution for form row atributes on a regular basis in all of my projects.
https://stackoverflow.com/questions/23011450/symfony-twig-how-to-add-class-to-a-form-row
That gives you all css hooks needed to make
{{ form_start(form) }} {{ form_widget(form) }} {{ form_end(form) }}
a viable way to render a form without any tweaking in the template.
Maybe that should be part of the standard symfony form component, but that would be another issue, i guess.

@nicolas-grekas nicolas-grekas modified the milestones: 3.4, 4.1 Oct 8, 2017
@nicolas-grekas nicolas-grekas modified the milestones: 4.1, next Apr 20, 2018
@xabbuh xabbuh added the Form label May 14, 2018
@symfony symfony deleted a comment from nicolas-grekas May 30, 2018
@xabbuh
Copy link
Member

xabbuh commented Jul 6, 2018

Thinking about this again the proposed solution looks good to me and is in line with the existing date_widget and time_widget options.

@mktcode Can you check the failure and make fabbot happy?

@mktcode
Copy link
Contributor Author

mktcode commented Jul 6, 2018 •
edited
Loading

@xabbuh @fabpot patch applied

@mktcode
Copy link
Contributor Author

mktcode commented Jul 31, 2018 •
edited
Loading

@xabbuh @fabpot will this PR ever be merged (or closed)? I would like to see my PR list empty again. :D

@xabbuh xabbuh requested a review from ogizanagi August 1, 2018 09:24
@fabpot
Copy link
Member

fabpot commented Aug 2, 2018

Thank you @mktcode.

@fabpot fabpot merged commit df19155 into symfony:master Aug 2, 2018
fabpot added a commit that referenced this pull request Aug 2, 2018
@fabpot
feature #21002 [Form] Added options for separate date/time labels in …
dd2f830 
…DateTimeType. (mktcode)

This PR was squashed before being merged into the 4.2-dev branch (closes #21002).

Discussion
----------

[Form] Added options for separate date/time labels in DateTimeType.

If your render date and time separately you need options for each label.

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| License       | MIT

Let's say you have the following form field:

```
$builder
    ->add('start', DateTimeType::class, [
        'date_widget' => 'single_text',
        ...
    ])
    ...
```
Then you can render the date and time widgets/rows/etc. separately:

```
<div>{{ form_row(form.start.date) }}</div>
<div>{{ form_row(form.start.time) }}</div>
```
But you can't provide labels for each, so what is displayed is just the uppercased field name ("Date" and "Time").

This PR adds 'date_label' and 'time_label' options, so you can do:
```
$builder
    ->add('start', DateTimeType::class, [
        'date_widget' => 'single_text',
        'date_label' => 'The Start Date',
        'time_label' => 'The Start Time',
        ...
    ])
    ...
```

Commits
-------

df19155 [Form] Added options for separate date/time labels in DateTimeType.
@mktcode mktcode deleted the patch-1 branch August 2, 2018 11:30
@javiereguiluz javiereguiluz mentioned this pull request Sep 7, 2018
Closed
@javiereguiluz
Copy link
Member

@mktcode thanks! We've created symfony/symfony-docs#10292 to not forget about documenting this new feature. If you want to give it a try, we can help you as much as you need in the Symfony Docs repository. Thanks!

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018
This was referenced Nov 3, 2018
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@sroze sroze sroze approved these changes

@xabbuh xabbuh xabbuh approved these changes

@ogizanagi ogizanagi Awaiting requested review from ogizanagi

Assignees
No one assigned
Labels
Feature Form Status: Reviewed
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.
9 participants
@mktcode @ogizanagi @xabbuh @fabpot @Shoplifter @javiereguiluz @sroze @nicolas-grekas @carsonbot