[#6995] - Use the Joomla way for bootstrap modals
- Closed
- 29 May 2015
- Medium
- Build: staging
- Easy Test
- # 6995
- Diff
- dgt41:Joomla_bootstrap_modal_
- Success The Travis CI build passed Details
User tests: Successful: Unsuccessful:
Exchange some class names
Most of the PRs that convert the mootools modals to bootstrap use JHtmlBootstrap::renderModal(etc)
but they should actually use JHtml::_('bootstrap.renderModal', etc).
And here is how I found out:
Me: JHtmlBootstrap::renderModal( vs JHtml::('bootstrap.renderModal'
The first one on my IDE with cmd + click on JHtmlBootstrap takes me to the correct file/class but the other takes me to JHtml and cmd + click on 'bootstrap.renderModal' isn’t really helpful as well. So I am a little bit confused here…
@mbabker: JHtml has some code that can let you override the various methods if called via JHtml::. Direct calling of JHtml (and its subclasses) methods break that functionality. The downside is just as you pointed out, IDE autocompletion is out the window.
So in order to prevent any future conflicts with this code is better to change it ASAP!
Because this is only a rename and some CS work I would like to ask the maintainers to just review and commit. @wilsonge @roland-d ?
To save other people and not to spent their time here, I provide a proof that all the relative modals still work as before. So:
administrator/components/com_contact/models/fields/modal/contact.php
administrator/components/com_content/models/fields/modal/article.php
administrator/components/com_menus/models/fields/menutype.php
administrator/components/com_menus/views/item/tmpl/edit_modules.php
administrator/templates/hathor/html/com_menus/menus/default.php
administrator/components/com_newsfeeds/models/fields/modal/newsfeed.php
administrator/components/com_templates/helpers/html/templates.php
administrator/components/com_users/helpers/html/users.php
administrator/templates/hathor/html/com_menus/menus/default.php
joomla-cms-bot
- | Labels |
Added:
?
|
||
zero-24
- | Category | ⇒ | Administration |
| Status | New | ⇒ | Pending |
| Easy | No | ⇒ | Yes |
@dgt41 Dimitris, I think you should check here too if the width and height options are needed: here some modals are indeed iframes (and thus they obey to those options), but I'm anyway unsure if it is a good idea (think of high-res displays, like retina displays: what is the behavior with those? I don't have one to check...)
| Milestone |
Added: |
||
Fix the conflicts please :)
@dgt41 Not sure if this is the right place.
When I add a new menu item, and click on the "Select menu type" button I got the following error:
Notice: Undefined variable: tispanle in C:\MAMP\htdocs\joomla-cms\administrator\components\com_menus\views\menutypes\view.html.php on line 108
Little bug? Or a problem in my local installation?
If I apply patch #6995 via Joomla! Patch Tester, I get a Fatal Error in :
administrator/components/com_menus/models/fields/menutype.php
Looks like it is a typo on line 89/90, a comma forgotten.
Corrected:
$html[] = JHtml::_(
'bootstrap.renderModal',
'menuTypeModal',
array(
I just wanted to be sure its been corrected. Thanks all for your quick reply.
@brianteeman and @ghazal Did you test this patch successfully?
@n9iels Could you test it again?
Looks like everything works fine.
@test looks good to me!
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/6995.
As this is effectively code style anyhow but also as there are 2 good tests I'm merging this :) Thanks!
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2015-05-29 10:09:27 |
| Closed_By | ⇒ | wilsonge |
johanjanssens
-
This is definitely a separate CS PR, but mentioning it here because I looked at the diff.
When you have a method call with arguments on multiple lines, you should have one argument per line and the arguments are indented one tab from the opening call. So, something like this: