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

New filter implementation damages all "collapse" elements #2784

Closed
udavka opened this issue Jun 21, 2019 · 8 comments
Closed

New filter implementation damages all "collapse" elements #2784

udavka opened this issue Jun 21, 2019 · 8 comments
Labels
Milestone

Comments

@udavka
Copy link

udavka commented Jun 21, 2019

Bug
After upgrading to 2.2 all "collapse" elements of the interface have been damaged.
Even the new filter form is damaged!

To reproduce
Get an example code (not accordion one) from https://getbootstrap.com/docs/4.0/components/collapse/ and put it anywhere on the list template.
Result:

  • in 2.1: everything works as expected, all collapsible content is collapsed from the beginning and could be displayed/hidden by a button click.
  • in 2.2: after the page opening all collapsible elements are displayed and could not be hidden. The content jerks only, but not collapses.

Additional effect
Elements of the new filter form cannot be collapsed as well - they have the same jerks, but not collapsing. As I see, they must work as an accordion. What is interesting: the accordion example from the Bootstrap site works perfectly.

Additional context
Tested in latest Chrome and Firefox.

@yceruto
Copy link
Collaborator

yceruto commented Jun 21, 2019

Thanks for reporting! I have researched a bit about this error with other libraries that include bootstrap plugins; If your project also includes it, then you should deactivate one when all plugins are initialized via data attributes.

I think the solution on this place should be instantiate the plugin via javascript with a custom target https://getbootstrap.com/docs/4.1/components/collapse/#via-javascript instead of via data attributes https://getbootstrap.com/docs/4.1/components/collapse/#via-data-attributes

I guess this code should be removed also:

$('.collapse').collapse();

I tried it but it didn't work :/ maybe @javiereguiluz can help here.

As workaround, you can add this line to the main js code in your project to avoid listen twice from collapse events:

// EasyAdminBundle already add the collapse plugins
// Kill one listener, so this will avoid listen from this event twice.
$(document).off('click.bs.collapse.data-api');

@yceruto yceruto added this to the 2.x milestone Jun 21, 2019
@yceruto yceruto added the bug label Jun 21, 2019
@javiereguiluz
Copy link
Collaborator

@udavka I've done what you asked: copy an example from Bootstrap collapse and put it in the list page. I could reproduce the first part of your bug report: collapse elements are not hidden initially. But I couldn't reproduce the other part: collapse elements work as expected.

If I change this in app.js, the first error is fixed:

-$('.collapse').collapse(); 
+$('.collapse').collapse('hide');

In practice:

collapse

@yceruto
Copy link
Collaborator

yceruto commented Jun 21, 2019

@javiereguiluz I'm guessing there is another import of the collapse plugins in project side, either through import 'bootstrap/js/src/collapse.js'; or import 'bootstrap';. @udavka can you confirm that?

Importing it twice, initializes two listeners on click.bs.collapse.data-api for [data-toggle="collapse"], that's why I can't collapse each filter element.
bug-collapsed

I tried to initialize our filter collapse via Javascript but it didn't worked to me.

@alterphp
Copy link
Contributor

alterphp commented Jun 22, 2019

I get the same bugs, every collapsible elements are broken.

@yceruto, @javiereguiluz, have you tried to include bootstrap-all.js from EasyAdmin (people using collapse before EasyAdmin filters required this file, as documented https://symfony.com/doc/master/bundles/EasyAdminBundle/book/design-configuration.html#loading-the-entire-bootstrap-framework). I think it's no more compatible with app.js... collapse.js is loaded twice, and so on (dropdown as well, same for CSS parts).

My opinion is that willing to split bootstrap into 2 files (features used by base EasyAdmin and other features of Bootstrap) is a bad idea. It saves some bandwidth for a one time loaded asset (HTTP cache will do the job). And it highly decreases DX on asset management, it limits the usage of Bootstrap for developers who use it at a deeper level.

If you really want to bring the lightest version of Bootstrap as possible, you should split bootstrap imports :

  • bootstrap-light.js
  • bootstrap-full.js

And aside, app.js that do not import any of them. Not good enough with webpack to know if it works well, but coupling app.js with an altered version of bootstrap is definitely inconsistent.

@javiereguiluz
Copy link
Collaborator

@udavka @alterphp could you please test if the changes made in #2787 would fix your problem? Thanks!

javiereguiluz added a commit that referenced this issue Jul 6, 2019
This PR was merged into the 2.0.x-dev branch.

Discussion
----------

Removed the bootstrap-all files

This implements the idea suggested by @alterphp in #2784. The new docs explain it:

```
Loading the Entire Bootstrap Framework
--------------------------------------

In EasyAdmin versions prior to 2.2.1, the backend didn't load the entire CSS and
JavaScript code from Bootstrap but only the parts that used it. This was made to
improve performance and required loading some separate files called
``bootstrap-all.css`` and ``bootstrap-all.js`` when you needed to use some
Bootstrap feature not included by default.

Given that the performance gain was minimal, this idea was abandoned and,
starting from EasyAdmin 2.2.1 the entire Boostrap CSS and JavaScript code is
loaded by default in all pages.
```

On my local computer, the compressed size of the entire Bootstrap contents is reasonable: 77KB for JS and 43KB for CSS.

Commits
-------

7784d92 Removed the bootstrap-all files
javiereguiluz added a commit that referenced this issue Jul 6, 2019
This PR was squashed before being merged into the 2.0.x-dev branch (closes #2787).

Discussion
----------

Removed the bootstrap-all files

This implements the idea suggested by @alterphp in #2784. The new docs explain it:

```
Loading the Entire Bootstrap Framework
--------------------------------------

In EasyAdmin versions prior to 2.2.1, the backend didn't load the entire CSS and
JavaScript code from Bootstrap but only the parts that used it. This was made to
improve performance and required loading some separate files called
``bootstrap-all.css`` and ``bootstrap-all.js`` when you needed to use some
Bootstrap feature not included by default.

Given that the performance gain was minimal, this idea was abandoned and,
starting from EasyAdmin 2.2.1 the entire Boostrap CSS and JavaScript code is
loaded by default in all pages.
```

On my local computer, the compressed size of the entire Bootstrap contents is reasonable: 77KB for JS and 43KB for CSS.

Commits
-------

2613970 Removed the bootstrap-all files
@javiereguiluz
Copy link
Collaborator

Hopefully fixed by #2787.

@udavka
Copy link
Author

udavka commented Jul 7, 2019

Sorry for the late response.
I have checked a new version, and it works perfectly, but... after the page opening all collapsible elements are still displayed. This occurs because of:

$('.collapse').collapse();

This line toggles all existing collapsible elements and everything that is normally closed - is toggled and opened. If you really need to have this line here to initialize collapsible elements which have no data-toggle="collapse", please rewrite it as $('.collapse').collapse({toggle:false}) (see https://getbootstrap.com/docs/4.0/components/collapse/#options)

@alterphp
Copy link
Contributor

Sorry for late update, my holidays are over.

@udavka is right. Manually enabling by Javacript should be avoided in a customizable context. Using data-attributes is the best default implementation.

I'm gonna PR this

javiereguiluz added a commit that referenced this issue Jul 15, 2019
…erphp)

This PR was merged into the 2.0.x-dev branch.

Discussion
----------

Remove manual initialization for collapse and dropdown

This fixes the last part of #2784.

Bootstrap does not more require manual initialization for **collapse** and **dropdown** components (popover still requires it).

Using `data-toggle="collapse|dropdown"` with manual JS initialization led to front misbehaviours.

Commits
-------

f875922 Remove manual initialization for collapse and dropdown
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants