Skip to content

Disable JS generation in JHtmlBoostrap::Modal()#6919

Closed
smz wants to merge 1 commit intojoomla:stagingfrom
smz:JHtmlBootstrapModal
Closed

Disable JS generation in JHtmlBoostrap::Modal()#6919
smz wants to merge 1 commit intojoomla:stagingfrom
smz:JHtmlBootstrapModal

Conversation

@smz
Copy link
Copy Markdown
Contributor

@smz smz commented May 10, 2015

This method was used by the old JHtmlBoostrap::renderModal() implementation.
Since the new implementation it is unneeded and the broken JS it was injecting could create isssues.

@smz
Disable JS generation in JHtmlBoostrap::Modal()
190e19f 
This method was used by the old JHtmlBoostrap::renderModal()
implementation.
Since the new implementation it is unneeded and the
broken JS it was injecting could create isssues.
@smz
Copy link
Copy Markdown
Contributor Author

smz commented May 10, 2015

As a case, please see #6918

If conecptually accepted, I don't think this need much testing...

@smz smz mentioned this pull request May 10, 2015
Closed
@Fedik
Copy link
Copy Markdown
Member

Fedik commented May 11, 2015

I would leave here static::framework(); for b/c reason,
because in theory some extension could call this method, but do not call boostrap.framework

@smz
Copy link
Copy Markdown
Contributor Author

smz commented May 11, 2015

TBH, I think anyone using JHtmlBoostrap::modal() just to load JQuery and bootstrap.min.js and then not using any of the other JHtmlBoostrap methods (all of them have a call to framework()), should burn in J! developer's hell (i.e. port Joomla! to another programming language of his/her choice bewteen COBOL, Lisp or APL), but if there is a consensus this must be done, I'll bow to the B/C god and re-instate it...

@mbabker
Copy link
Copy Markdown
Contributor

mbabker commented May 11, 2015

Personally, I'm fine with this as is.

@smz
Copy link
Copy Markdown
Contributor Author

smz commented Jun 10, 2015

@mbabker, @DGT41: can you please test this?

@dgrammatiko
Copy link
Copy Markdown
Contributor

dgrammatiko commented Jun 10, 2015

@smz Does this one really needs a test?
@test ok

@smz
Copy link
Copy Markdown
Contributor Author

smz commented Jun 10, 2015

@smz Does this one really needs a test?

Apparently...

@smz
Copy link
Copy Markdown
Contributor Author

smz commented Jun 10, 2015

... remember the test flag in http://issues.joomla.org/tracker/joomla-cms/6919 or I'm afraid it will not be taken into account...

@anibalsanchez
Copy link
Copy Markdown
Contributor

anibalsanchez commented Jun 13, 2015

#test confirmed. JHtmlBootstrap::modal() does nothing!

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/6919.

@smz
Copy link
Copy Markdown
Contributor Author

smz commented Jun 16, 2015

We have 2 test: RTC?

@Bakual Bakual added this to the Joomla! 3.5.0 milestone Jun 16, 2015
@Bakual Bakual added the RTC This Pull Request is Ready To Commit label Jun 16, 2015
@Bakual
Copy link
Copy Markdown
Contributor

Bakual commented Jun 16, 2015

Setting RTC. Probably should go into 3.5.

@smz
Copy link
Copy Markdown
Contributor Author

smz commented Jun 19, 2015

I think this can go with 3.4.2: the deprecation was already there at 3.4.0 and this just eliminate some useless and potentially dangerous Javascript.

... but that isn't much important either, at least to me! 😄

@mbabker
Copy link
Copy Markdown
Contributor

mbabker commented Jul 11, 2015

Merged to 3.5-dev via 4ab5b3b

@smz
Copy link
Copy Markdown
Contributor Author

smz commented Jul 11, 2015

Thanks, @mbabker!

@smz smz deleted the JHtmlBootstrapModal branch July 11, 2015 14:29
roland-d pushed a commit that referenced this pull request Aug 6, 2015
@smz @roland-d
Disable JS generation in JHtmlBoostrap::modal() (Fix #6919)
df4b45f
@zero-24 zero-24 removed the RTC This Pull Request is Ready To Commit label Oct 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Joomla! 3.5.0

Development

Successfully merging this pull request may close these issues.

7 participants

@smz @Fedik @mbabker @dgrammatiko @anibalsanchez @Bakual @zero-24