AliHichem / AliDatatableBundle

Symfony2 Ajax Datagrid Bundle for doctrine2 entities
MIT License
112 stars 71 forks source link

Fix incorrect script container ID #117

Closed lushc closed 9 years ago

lushc commented 9 years ago

Looking at this line in the DatatableListener it seems we should be able to control the position of the injected JS, e.g. before our own template scripts so we can manipulate the datatable after it has been initialized:

{% block javascripts %}
  {{ parent() }}
  {{ include('AliDatatableBundle:Internal:container.html.twig') }}
  <script type="text/javascript">
    $(document).ready(function () {
      var opts = $('#dta-unique-id_1').dataTable().fnSettings();
      console.log(opts); // do whatever
    });
  </script>
{% endblock %}

However the ID used in the template and the event listener are mismatched, which breaks this use-case.

AliHichem commented 9 years ago

The file "AliDatatableBundle:Internal:container.html.twig" is actually an obsolete file that I forgot to remove, it's never used anywhere in the code ... ( I just removed it in v2.0.1)

Looking at this line in the DatatableListener it seems we should be able to control the position of the injected

I'm not sure how you get that assumption, controlling the position of the injected js is something done in the bundle internally and should not be exposed to the developer.

lushc commented 9 years ago

Personally I don't see how something introduced in 2.0 and removed in 2.0.1 can be classed as obsolete. The function is clearly checking whether the script container is already present in the DOM, and only creates a new container at the end of the DOM if it's position is not found. And with a template it implied that the proper method of controlling injection location was through a Twig include.

If you want to remove this feature you'll have to refactor the function itself, but I recommend against it since there is a valid use case for manipulating a datatable object after it's been initialized.

AliHichem commented 9 years ago

That file "was" used "while" creating v2 and I went to write the dom manually in the listener instead since it has only one line and when releasing v2 that file was "never" used in the code. So again as I "said" that file is "obsolete" and since I forgot to remove it when creating V2.0.0 I think it make perfect sense to remove it after that and mark the change under V2.0.1 .

Since you like to dig into code source if you checkout v2.0.0 and make a search on "AliDatatableBundle:Internal:container.html.twig" you will see that that file is NEVER used anywhere in the code source that MEANS it 's NO feature unlike what you "assumed".

Please try at least to understand the source code before making a pull request or creating an issue.

lushc commented 9 years ago

Wow, okay, "please try at least to understand the source code"? Quite insulting when I think it's quite clear from the implementation that a check is made for the position of the script container in the DOM and only inserts it at the end of the body if it isn't already present! Yes the template was obviously unused (as I discovered when trying to fit my use-case), but based on the function's behaviour and the fact the file was in the bundle, it lead me to believe it could have been an undocumented (but broken) feature. After all, it's quite common for a bundle to give you control over where its JS is injected.

Hence the pull request, as I could not possibly ascertain your intent otherwise.