Skip to content
This repository was archived by the owner on Sep 16, 2021. It is now read-only.

Conversation

wouterj
Copy link
Member

@wouterj wouterj commented Aug 26, 2013

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #5
License MIT
Doc PR -

Todo

  • Rename files
  • Update CS
    • admintree.js
    • selecttree.js
  • Add default values
    • admintree.js
    • selecttree.js
  • Concat name on 18th character and show full name in tooltip
  • Do not show link if not editable

Copy link
Member Author

Choose a reason for hiding this comment

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

this are all options. null should get replaced with their default value.

Copy link
Member

Choose a reason for hiding this comment

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

is this a note to yourself? if these where previously undefined when not configured, we might simply validate and throw better errors. for some, there are sensible defaults for sure, but the ajax section for example does not have any sensible defaults.

@lsmith77
Copy link
Member

lsmith77 commented Sep 4, 2013

@wouterj do you have time before lunch to fix this up?

@wouterj
Copy link
Member Author

wouterj commented Sep 4, 2013

@lsmith77 no, sorry. I'm pretty bussy the last week. I think it's safe to merge this PR if it's really needed, AFAICS nothing of the logic is changed.

@dbu
Copy link
Member

dbu commented Sep 4, 2013

as far as i can tell, its cleanup and no important fixes. so i'd prefer to not merge and wait until it really is clean. adding the initializing with null worries me as i don't really understand the implications.

@dbu
Copy link
Member

dbu commented Oct 5, 2013

@wouterj is there a part of this that we can merge this weekend? i am still worried by the null initialization as i don't know what it means...

@wouterj
Copy link
Member Author

wouterj commented Oct 5, 2013

I've reverted the defaults and updated the CS for the select_tree. Let's merge this now (if it's ok) and after that, I'll try to submit a fix for the bugs this weekend

@dbu
Copy link
Member

dbu commented Oct 5, 2013

i am +1 to merge this in. did you test it in the sandbox and things still work?

/cc @lsmith77

@wouterj
Copy link
Member Author

wouterj commented Oct 5, 2013

no, didn't test it (only linted it). I'll do it this afternoon on the sandbox.

@wouterj
Copy link
Member Author

wouterj commented Oct 5, 2013

tested it and I can't find any problems

lsmith77 added a commit that referenced this pull request Oct 5, 2013
[WIP] Clean up JavaScript scripts
@lsmith77 lsmith77 merged commit 62e60e6 into symfony-cmf:master Oct 5, 2013
@wouterj wouterj deleted the issue_5 branch October 5, 2013 20:33
lsmith77 added a commit that referenced this pull request Oct 5, 2013
Fixed BC break introduced in #45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants