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

Issue89fix #92

Merged
merged 21 commits into from
Dec 30, 2016
Merged

Issue89fix #92

merged 21 commits into from
Dec 30, 2016

Conversation

sdutry
Copy link
Contributor

@sdutry sdutry commented Dec 23, 2016

fixes #89

This pull request fixes several javascript problems, which, in most cases, were caused by the order in which they were loaded.

These problems were only happening with the following settings:

  • loadAtOnce="false"
  • loadFromGoogle="false"

I'd like someone else to merge this pull request. That way someone else will have looked at the code and might have spotted possible new bugs.

jogep and others added 19 commits December 6, 2016 21:42
…i exists for the other javascripts. Also removed the locations where it would be loaded dynamicaly
…minified files for plugin javascripts from the git repository and adding them to the generation process in the pom
@sdutry sdutry requested review from jogep and flofourcade December 23, 2016 14:11
@sdutry sdutry added this to the 4.0.2 milestone Dec 23, 2016
@sdutry sdutry added the bug label Dec 23, 2016
Copy link
Contributor

@jogep jogep left a comment

Choose a reason for hiding this comment

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

Thank you for taking care of this issue.

@@ -56,3 +56,11 @@ src/main/resources/template/js/base/widget.min.js
/src/main/resources/template/js/base/checkboxradio.min.js
/src/main/resources/template/js/base/form-reset-mixin.min.js
/src/main/resources/template/i18n/jquery-ui-timepicker-*.min.js

Copy link
Contributor

Choose a reason for hiding this comment

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

Why you ignore this resources?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jogep
My mistake. i must have accidently ignored the full files instead of the minified versions.

I'll fix it ASAP

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jogep should be fixed now

@@ -23,6 +23,8 @@
<#assign jQueryVersion="2.2.4">
</#if>
<#assign jQueryUIVersion="1.12.1">
<#-- issue89: temporary fix because of i18n files not being available for current jQuery UI version -->
Copy link
Contributor

Choose a reason for hiding this comment

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

Does anyone know why 1.12.1 i18n resources are not available via google cdn?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jogep
Personaly i don't have a clue, this was the only fix i could think of for now.

I didn't see any message by them saying where they're located either

Copy link
Contributor

@flofourcade flofourcade left a comment

Choose a reason for hiding this comment

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

Great ! I'll take care about the possible issue on datatables you've talked about soon.

@sdutry sdutry merged commit aa7d8db into struts-community-plugins:release/4.0.2 Dec 30, 2016
@sdutry sdutry deleted the issue89fix branch December 30, 2016 19:46
@jogep
Copy link
Contributor

jogep commented Dec 30, 2016

@sdutry @flofourcade
Thank you both for taking care of this issue. What you both think, can I release a new version beginning of next year with the fixes for this issue?

@sdutry
Copy link
Contributor Author

sdutry commented Dec 31, 2016

@jogep
given the possible extent of this bug, I do think it would be a good to have a new version with this fix.

@flofourcade
Copy link
Contributor

@jogep
Once we re sure this issue is fixed I think we should go for release ASAP as it affects the default behaviour of the head tag and many users are probably impacted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants