Skip to content

HTML5 range documentation#5458

Merged
weaverryan merged 5 commits intosymfony:2.8from
harikt:range
Jul 16, 2015
Merged

HTML5 range documentation#5458
weaverryan merged 5 commits intosymfony:2.8from
harikt:range

Conversation

@harikt
Copy link
Copy Markdown
Contributor

@harikt harikt commented Jun 28, 2015

Hi,

Q A
Doc fix? yes
New feature? yes
BC breaks? no
Deprecations? no
Fixed tickets [#5439]
Code PR [12607]
License MIT

I am trying to contribute to range added for the PR symfony/symfony#12067 & symfony/symfony#11979 which fixes #5439 .

Need some help on moving for I am really new to the documentation.

The current issues I feel is whether we need to add Basic usage or is that only reference ?

I had a look at http://symfony.com/doc/current/reference/forms/types/number.html which would have been similar, but that seems it referencing to choice seems more confusing.

I have looked into other types also and almost all are some what similar.

Any input is highly appreciated and will work on in the free time. If someone is interested please do take the PR or finish it .

Thank you.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hm. Anyone have idea how many spaces should be kept ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You need one more in this and in the next line (have a look at the right most | character).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

By the way, min and max are no options of the form type. You need to pass them as attr option keys.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

may be we need attr here.

@harikt
Copy link
Copy Markdown
Contributor Author

harikt commented Jun 28, 2015

Hey @xabbuh ,

Thanks for all the comments. What about the examples ?

@xabbuh
Copy link
Copy Markdown
Member

xabbuh commented Jun 28, 2015

I think a simple example showing how to configure the min and max attributes should be enough (see, for example, the test cases in the code pull request: https://github.com/symfony/symfony/pull/12067/files#diff-e11656489f1e01413320bd5edff0f0c7R1350).

@harikt
Copy link
Copy Markdown
Contributor Author

harikt commented Jun 28, 2015

Hey @xabbuh ,

May be I didn't explained better regarding what I was referencing to.

Basically if you check the markdown of http://symfony.com/doc/current/reference/forms/types/number.html ( https://raw.githubusercontent.com/harikt/symfony-docs/range/reference/forms/types/number.rst ) , you can see the number was not having the correct examples. It refers to hidden, choice fields etc for data, empty_data etc .

I will make the changes and get back.

Thank you.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please add a code-block directive before the example code:

.. code-block:: php

    $builder->add('name', 'range', array(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

sure. Thanks.

@snoek09
Copy link
Copy Markdown

snoek09 commented Jun 29, 2015

The new range documentation isn't included anywhere yet.
Please add it to the list in /reference/forms/types.rst

I found these small issues by testing the documentation first on my local machine:
http://symfony.com/doc/current/contributing/documentation/format.html#testing-documentation

@harikt harikt changed the title [WIP] Starting with range documentation HTML5 range documentation Jul 3, 2015
@harikt
Copy link
Copy Markdown
Contributor Author

harikt commented Jul 3, 2015

@xabbuh @snoek09 thank you guys for your time and review.

Have updated the changes.

Thank you.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This description should be moved before the mapped option's description.

@xabbuh
Copy link
Copy Markdown
Member

xabbuh commented Jul 4, 2015

Very great @harikt. I left you one minor comment. After that I think this looks ready to be merged.

@harikt
Copy link
Copy Markdown
Contributor Author

harikt commented Jul 4, 2015

@xabbuh done 👍 .

Thank you.

@xabbuh
Copy link
Copy Markdown
Member

xabbuh commented Jul 4, 2015

👍

@weaverryan
Copy link
Copy Markdown
Contributor

@harikt! This is wonderful! I have no changes - and now I can also close the related issue :). Thanks!

@weaverryan weaverryan merged commit 2863079 into symfony:2.8 Jul 16, 2015
weaverryan added a commit that referenced this pull request Jul 16, 2015
This PR was merged into the 2.8 branch.

Discussion
----------

HTML5 range documentation

Hi,

| Q                      | A
| ------------- | ---
| Doc fix?            | yes
| New feature?   | yes
| BC breaks?      | no
| Deprecations? | no
| Fixed tickets    | [#5439]
| Code PR      | [12607]
| License       | MIT

I am trying to contribute to range added for the PR symfony/symfony#12067 & symfony/symfony#11979 which fixes #5439 .

Need some help on moving for I am really new to the documentation.

The current issues I feel is whether we need to add Basic usage or is that only reference ?

I had a look at http://symfony.com/doc/current/reference/forms/types/number.html which would have been similar, but that seems it referencing to choice seems more confusing.

I have looked into other types also and almost all are some what similar.

Any input is highly appreciated and will work on in the free time. If someone is interested please do take the PR or finish it .

Thank you.

Commits
-------

2863079 Fix moving the mapped down as per @xabbuh
abf3e8e Fix the rendering issue
3c6ff76 Fix issues reported by @snoek09 . Thank you.
a2c3f21 Make the necessary changes mentioned by @xabbuh and add example for form
d4d1f12 Starting with range documentation
@harikt harikt deleted the range branch July 16, 2015 04:47
@harikt
Copy link
Copy Markdown
Contributor Author

harikt commented Jul 16, 2015

cool. Thank you all.

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.

4 participants