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

[bugfix] Prioritize jQuery UI scripts in "assets.javascripts" config in order to avoid initialization issues #5087

Merged
merged 1 commit into from
Jun 18, 2018

Conversation

phansys
Copy link
Member

@phansys phansys commented Apr 23, 2018

I am targeting this branch, because this guards BC.

Changelog

### Changed
- Default load order of `assets.javascripts` at `Configuration::getConfigTreeBuilder()`
### Fixed
- ECMAScript error `jquery.js:250 Uncaught Error: cannot call methods on button prior to initialization;` while trying to use `$.fn.button()` (ref: https://github.com/twbs/bootstrap/issues/6094)

Todo:

  • Check if changes made at 359ce81 can be reverted (using noConflict() instead).

@phansys phansys force-pushed the configuration_javascripts branch from bfdb9a2 to 41c1232 Compare April 23, 2018 17:25
kunicmarko20
kunicmarko20 previously approved these changes Apr 23, 2018
Copy link
Contributor

@kunicmarko20 kunicmarko20 left a comment

Choose a reason for hiding this comment

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

I guess we can't know if this will break something else

@phansys
Copy link
Member Author

phansys commented Apr 23, 2018

I'm afraid we can't.

@jordisala1991
Copy link
Member

#5047

we changed this a few PRs ago... does this PR revert the older one?

@phansys
Copy link
Member Author

phansys commented Apr 23, 2018

does this PR revert the older one?

That seems to be true. Then, I'll try to make some tests around noConflict() approach and will be back with the results. Thank you for pointing the previous change ;)

Copy link
Member

@jordisala1991 jordisala1991 left a comment

Choose a reason for hiding this comment

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

Prevent accidental merge

@kunicmarko20
Copy link
Contributor

and it looked familiar but I wasn't sure 😅

@phansys phansys changed the title [bugfix] Prioritize jQuery UI scripts in "assets.javascripts" config in order to avoid initialization issues [bugfix] [WIP] Prioritize jQuery UI scripts in "assets.javascripts" config in order to avoid initialization issues Apr 24, 2018
@phansys
Copy link
Member Author

phansys commented Apr 24, 2018

@verheyenkoen, are you sure the only way to fix the issue you've reported on #5047 is to change the order of the scripts? AFAIK, the embedded version of jQuery doesn't support noConflict() (Bootstrap does, see twbs/bootstrap#6094 (comment)), so with your PR, I'm not able to use features from button: jquery.js:250 Uncaught Error: cannot call methods on button prior to initialization;.
Thank you in advance.

@verheyenkoen
Copy link
Contributor

@phansys I don't know. This is a fix I found. If it's no good I guess we have to revert it.

@phansys
Copy link
Member Author

phansys commented Apr 24, 2018

Ok, let me do some checks in order to determine how we could make both fixes to co-exist.

@phansys
Copy link
Member Author

phansys commented Apr 24, 2018

AFAIK, there is no way to solve both issues without reverting the changes at #5047 and using some of the alternatives proposed on SO (https://stackoverflow.com/a/20391739 by instance). Moreover, I don't know if the only bug that PR fixes is the visibility of the close button or if there are other issues covered by that modification.

@jordisala1991
Copy link
Member

Any idea on how to deal with this one @greg0ire ?

@greg0ire
Copy link
Contributor

Pick the lesser of the 2 bugs?

@jordisala1991
Copy link
Member

Then, probably this bug is bigger than the other? @phansys the StackOverflow you linked covers the other bug report?

@jordisala1991
Copy link
Member

Please edit the docs like the other PR: https://github.com/sonata-project/SonataAdminBundle/pull/5047/files#diff-e252be027e26148c11d971dc969f4be0R205

@phansys
Copy link
Member Author

phansys commented Apr 25, 2018

the StackOverflow you linked covers the other bug report?

I think not, because I don't know how to provide the button.noConflict() instance to $(selector).dialog().

In my particular use case, my app was using $(selector).button('loading'), $(selector).button('reset'), etc; and the change introduced in #5047 broke these calls.

Update at configuration.rst is done.

jordisala1991
jordisala1991 previously approved these changes Apr 25, 2018
@phansys phansys changed the title [bugfix] [WIP] Prioritize jQuery UI scripts in "assets.javascripts" config in order to avoid initialization issues [bugfix] Prioritize jQuery UI scripts in "assets.javascripts" config in order to avoid initialization issues Apr 25, 2018
@jordisala1991
Copy link
Member

We might merge this one after all

@OskarStark OskarStark requested a review from kunicmarko20 June 18, 2018 18:06
Copy link
Contributor

@kunicmarko20 kunicmarko20 left a comment

Choose a reason for hiding this comment

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

Merge it and wait for people to scream?

@OskarStark
Copy link
Member

Merge it and wait for people to scream?

Yes 👍

@OskarStark OskarStark merged commit 82a4d6e into sonata-project:3.x Jun 18, 2018
@OskarStark
Copy link
Member

Thank you @phansys

@phansys phansys deleted the configuration_javascripts branch June 18, 2018 19:05
pestaa pushed a commit to Crosssec/SonataAdminBundle that referenced this pull request Aug 6, 2018
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