Skip to content

Update form_collections.rst #3157

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

Merged
merged 2 commits into from
Nov 12, 2013
Merged

Update form_collections.rst #3157

merged 2 commits into from
Nov 12, 2013

Conversation

attardi
Copy link
Contributor

@attardi attardi commented Nov 5, 2013

The assignment to collectionHolder must be done within jQuery ready function, or else it will be null.
BTW, the variables in JavaScript do not need the $, which is confusing since in jQuery $ has a special meaning.

The assignment to collectionHolder must be done within jQuery ready function, or else it will be null.
BTW, the variables in JavaScript do not need the $, which is confusing since in jQuery $ has a special meaning.
@wouterj
Copy link
Member

wouterj commented Nov 5, 2013

I always use $ to indicate it's a jQuery object, but if it's confusing I agree with removing them

@wouterj
Copy link
Member

wouterj commented Nov 5, 2013

so 👍

(could you please include the PR template into your PR description?)

@attardi
Copy link
Contributor Author

attardi commented Nov 6, 2013

Actually the variable collectionHolder should be assigned within the jQuery ready function, but declared outside, since it is used in other functions as well.

@weaverryan
Copy link
Member

Hi Giuseppe!

It's subjective, but I don't agree with removing the $ in front of the variables. I believe it's a semi-standard - http://stackoverflow.com/questions/205853/why-would-a-javascript-variable-start-with-a-dollar-sign#answer-553734. We'll see what others think. But you're right about the variable move :).

Cheers!

@bicpi
Copy link
Contributor

bicpi commented Nov 6, 2013

Using $ in front is also a best practice for me; at first glance you know what kind of variable it is.

@ggam
Copy link

ggam commented Nov 6, 2013

In case we keep the $ prefix, we can add a notice explaining its meaning.

@weaverryan
Copy link
Member

That sounds like a good suggestion to me. @attardi can you make the change to put the $ back and add a note about it?

Thanks!

@lyrixx
Copy link
Member

lyrixx commented Nov 9, 2013

I used to prefix my jQuery object with $ too.

@attardi
Copy link
Contributor Author

attardi commented Nov 9, 2013

On 11/9/2013 18:59, Ryan Weaver wrote:

That sounds like a good suggestion to me. @attardi
https://github.com/attardi can you make the change to put the $ back
and add a note about it?

Thanks!

I did it.
However github generated another branch when I tried to edit.
Then I closed and it yold me I had unmerged pull requests.
I am not sure I did the right thing.

-- Beppe

@wouterj
Copy link
Member

wouterj commented Nov 9, 2013

@attardi you should go to your repo, select the patch-1 branch and do the updates then.

The variable collectionHolder must be assigned inside the jQuery ready function, but must be declared outside, since it is used also in other functions.

I have put back the '$' in front of names of variables having jQuery objects as values and for uniformity, also added it to collectionHolder.
@attardi
Copy link
Contributor Author

attardi commented Nov 9, 2013

On 11/9/2013 19:47, Wouter J wrote:

@attardi https://github.com/attardi you should go to your repo,
select the patch-1 branch and do the updates then.


Reply to this email directly or view it on GitHub
#3157 (comment).

Did it.
I changed the name of collectionHolder to $collectionHolder, because
that too is a jQuery variable.

-- Beppe

weaverryan added a commit that referenced this pull request Nov 12, 2013
Update form_collections.rst
@weaverryan weaverryan merged commit bb34344 into symfony:2.0 Nov 12, 2013
weaverryan added a commit that referenced this pull request Nov 12, 2013
@weaverryan
Copy link
Member

Thanks everyone! Small tweak at sha: ae5a6d2

Cheers!

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.

6 participants