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

Add dateinterval type reference #4817

Merged
merged 1 commit into from
Nov 10, 2016

Conversation

MisatoTremor
Copy link
Contributor

@MisatoTremor MisatoTremor commented Jan 13, 2015

Q A
Doc fix? no
New docs? yes
Applies to 3.2
Fixed tickets #4816

| with_seconds | ``Boolean`` | The value of the `with_seconds`_ option. |
+--------------+-------------+-----------------------------------------------------------------+
| with_invert | ``Boolean`` | The value of the `with_invert`_ option. |
+--------------+-------------+-----------------------------------------------------------------+
Copy link
Member

Choose a reason for hiding this comment

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

I would use the "simple" table syntax:

==============  ===========  ======================================
Variable        Type         Usage
==============  ===========  ======================================
``widget``      ``mixed``    The value of the `widget`_ option.
``with_years``  ``boolean``  The value of the `with_years`_ option.
and             so           on
==============  ===========  ======================================

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@MisatoTremor
Copy link
Contributor Author

Changed it like @javiereguiluz said.
I only swapped the two RFC duration strings, putting the shorter (and hopefully easier to understand) one in the intro text and the full example in the string input description.
Also fixed the sorting of the options list to match the other type docs.

@xabbuh xabbuh added this to the 3.2 milestone May 21, 2016
fabpot added a commit to symfony/symfony that referenced this pull request Jun 22, 2016
…m type (MisatoTremor)

This PR was merged into the 3.2-dev branch.

Discussion
----------

[Form][FrameworkBundle][Bridge] Add a DateInterval form type

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #13389
| License       | MIT
| Doc PR        | symfony/symfony-docs#4817

Replaces #15030

Commits
-------

f7669be [Form] Add a DateInterval form type Also add dateinterval widget to twig templates.
@HeahDude
Copy link
Contributor

symfony/symfony#16809 (comment) is merged now, can a maintainer please remove "On hold" label?

@MisatoTremor What the status here?

@MisatoTremor
Copy link
Contributor Author

@HeahDude
Updated for 3.2

@xabbuh xabbuh removed the On hold label Jun 23, 2016
@xabbuh xabbuh changed the title [WCM] Add dateinterval type reference Add dateinterval type reference Jun 23, 2016
| Options | - `days`_ |
| | - `placeholder`_ |
| | - `hours`_ |
| | - `input`_ |
Copy link
Member

Choose a reason for hiding this comment

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

looks like something went wrong here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed read_only field reference, build now runs successfully.

@xabbuh What do you mean with this line comment?

Copy link
Member

Choose a reason for hiding this comment

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

@MisatoTremor When I reviewed this earlier today the diff looked completely broken. Looks like this was related to my browser then or the issue disappeared with your push. So everything's fine here how. :)


DateIntervalType Field Type
===========================

Copy link
Member

Choose a reason for hiding this comment

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

missing a versionadded directive:

.. versionadded:: 3.2
   The DateIntervalType field type was introduced in Symfony 3.2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

@wouterj
Copy link
Member

wouterj commented Jul 2, 2016

👍 Thanks for writing the documentation for the feature @MisatoTremor!

status: reviewed

date interval. It can be rendered as a integer or text input or select tags.

The underlying format of the data can be a ``DateInterval`` object,
a `RFC 3339`_ duration string (e.g. ``P1DT12H``) or an array.
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure we want to link to the RFC 3339? In that RFC you can read the following:

rfc_3339

It looks like we should link instead to ISO 8601 standard: https://en.wikipedia.org/wiki/ISO_8601

@javiereguiluz
Copy link
Member

@MisatoTremor thanks for adding this form type and for providing the docs too. Today we've highlighted it in the Symfony blog (see New in Symfony 3.2: DateInterval form type).

When writing the blog post, I wondered if this type could be a bit easier to use. In particular, I wonder if the input option could be removed. If it's only used to tell the form type which format we are using (DateInterval, array or string) I think we can detect the format automatically. Do you think that would be possible? Thanks!

@MisatoTremor
Copy link
Contributor Author

@javiereguiluz Thanks for the nice highlight! :)

I've included the option according to other date/time types, but this indeed sounds like a great improvement. Maybe this could be done for the other types, too. Will look into into

options are ``input`` and ``widget``.

Suppose that you have a ``remindEvery`` field whose underlying interval is a
``DateInterval`` object. The following configures the ``dateinterval`` type
Copy link
Member

Choose a reason for hiding this comment

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

... configures the ``DateIntervalType``

@weaverryan
Copy link
Member

Thanks for the work @MisatoTremor - it's a really interesting component! And I'm sorry, I just peppered you with comments :)

Status: Needs Work

@MisatoTremor
Copy link
Contributor Author

@weaverryan Thank for your review Ryan. I applied all but one which I commented upon.

@wouterj
Copy link
Member

wouterj commented Nov 8, 2016

Hi @MisatoTremor! I'm sorry for the long delay, seems like this one slipped under our radar. As 3 doc members already reviewed this and you fixed their comments, I assume this one is ready to merge.

/cc @symfony/team-symfony-docs

status: reviewed

@javiereguiluz
Copy link
Member

👍 this is indeed ready to be merged. Thanks @MisatoTremor.

@MisatoTremor
Copy link
Contributor Author

@wouterj I totally understand this. :) There's much to do and also more important sections. You all are doing a great job!

Thanks for the update.

@wouterj
Copy link
Member

wouterj commented Nov 10, 2016

Thanks Steffen for your work & patience! This is now merged in the docs and will be online tomorrow.

@wouterj wouterj merged commit 3d55585 into symfony:master Nov 10, 2016
wouterj added a commit that referenced this pull request Nov 10, 2016
This PR was merged into the master branch.

Discussion
----------

Add dateinterval type reference

| Q | A |
| --- | --- |
| Doc fix? | no |
| New docs? | yes |
| Applies to | 3.2 |
| Fixed tickets | #4816 |

Commits
-------

3d55585 Add dateinterval type reference
@MisatoTremor MisatoTremor deleted the add_dateinterval_doc branch November 22, 2016 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants