? ? Success

User tests: Successful: Unsuccessful:

avatar JoomliC
JoomliC
4 May 2016

This PR introduces bootstrap-tooltip-extended.js to add 4 positions and RTL support for BS tooltips.

It follows discussion about tooltip placement issue when inside modals: #10100 (comment)

Summary of Changes

  • New JHtml::_('bootstrap.tooltipExtended'); method to load bootstrap-tooltip-extended.js to get 4 extra positions : top-left, top-right, bottom-left and bottom-right (https://github.com/cyrilreze/bootstrap-tooltip-extended)
  • Add tooltip-extended arrow css styles in bootstrap-extended.less
  • Define a default data-placement="auto-dir top-left" (if data-placement is undefined) for class .modalTooltip (handler class for BS tooltip when inside a modal-body).
  • In the same time, remove previously changes for Batch modals, when adding tooltip container in PR #9944 and #9379 (this is not needed anymore since the container is now set directly in modal layout main, PR #10100)

Testing Instructions

  • Apply Patch and empty cache
  • Open any modals not using iframe: Batch, Copy Template...
  • Check if tooltip placement is top-left (as shown in screenshot below)
  • Change language to a RTL language (eg. Arabic)
  • Then open again the same modals as in LTR language : the position of tooltip is reversed (top-left becomes top-right)

Screenshots

(eg. batch modules)

BEFORE PATCH
capture d ecran 2016-05-05 a 15 50 02

AFTER PATCH
Left-to-right (LTR) language direction :
capture d ecran 2016-05-04 a 17 01 16

Right-to-left (RTL) language direction :
capture d ecran 2016-05-04 a 17 26 29

avatar JoomliC JoomliC - open - 4 May 2016
avatar JoomliC JoomliC - change - 4 May 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 4 May 2016
Labels Added: ?
avatar brianteeman brianteeman - change - 4 May 2016
Category External Library Feature Request JavaScript Templates (admin) Templates (site)
avatar JoomliC JoomliC - change - 4 May 2016
The description was changed
avatar andrepereiradasilva
andrepereiradasilva - comment - 4 May 2016

Did you change bootstrap js and bootstrap extended css?
Are they not vendor libraries that should not be changed?

I think you should had as new files.

avatar mbabker
mbabker - comment - 4 May 2016

bootstrap-extended IIRC is a Joomla thing, not native Bootstrap.

And the bootstrap.js is already core hacked to bits to deal with event names, MooTools incompatibilities, and "auto-hover" on the dropdown menus (compare joomla.org and api.joomla.org mega menus and how you have to click to move between them; the former is using Joomla Bootstrap and the later the native Bootstrap). So, we're already long past pure vendor files (at least here).

avatar andrepereiradasilva
andrepereiradasilva - comment - 4 May 2016

ok, didn't know the history.
I tought any vendor code should not be "hacked".

avatar mbabker
mbabker - comment - 4 May 2016

The keyword there is "should" ????

It really should be "must", but considering Joomla has a history of hacking third party code, good luck changing that.

avatar brianteeman
brianteeman - comment - 4 May 2016

When bs abandoned v2 and we were stuck with it we had no option


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

avatar andrepereiradasilva
andrepereiradasilva - comment - 4 May 2016

i see, vendor code MUST not ever be changed ... for 4.0 then

avatar mbabker
mbabker - comment - 4 May 2016

Most of those hacks came when we first implemented Bootstrap 2.1 because they weren't going to make changes to deal with the MooTools conflicts and someone decided they wanted fluid dropdowns on the top nav bar in Isis (which I agree is a nice little UX thing but it came at the expense of hacking Bootstrap).

avatar andrepereiradasilva
andrepereiradasilva - comment - 4 May 2016

ok, so, for what is to this PR, this change in vendor lib is ok, because the vendor lib is already changed.
fine, to me.

avatar JoomliC
JoomliC - comment - 4 May 2016

Note: i can add this as a separated plugin too. ;-)

The first reason is to have a better placement of the tooltip when inside a modal. BS 3 has an auto placement based on an additional viewport option (which solves such issue, but we are in BS2).
This extension plugin here works both on version 2 and 3 of Bootstrap.

We can too implement tooltip from BS3 (backward compatible) to replace BS 2 tooltip plugin, and then just set the viewport when tooltip inside a modal, to the modal ID ;-)

avatar JoomliC
JoomliC - comment - 4 May 2016

Did you change bootstrap js and bootstrap extended css?
Are they not vendor libraries that should not be changed?

@andrepereiradasilva note about bootstrap, i don't have hacked nor changed nothing in Bootstrap, just added a plugin in bootstrap.js, which extends tooltip plugin.
bootstrap.js is a one readable compiled file of all bootstrap plugins as you can customize it yourself on getbootstrap: http://getbootstrap.com/customize/#plugins
bootstrap-tooltip-extended.js is just one plugin, and nothing change in library bootstrap-tooltip.js
So here, no hacking of bootstrap, only extending. ;-)

I think you should had as new files.

Maybe an idea...! :+1:
As this way, we can call it only when needed, and could be used by site template using bootstrap 3.

With something like :
JHtml::_('bootstrap.tooltipExtended', $selector, $params = array());

OR to extend tooltip class like this :
JHtml::_('bootstrap.tooltip', $selector, $params = array(), $extended = false);
and then loading separated js and css files for tooltip extended when needed ?

@andrepereiradasilva, @mbabker, @brianteeman ...
What's your opinion about this other possibility ?

avatar andrepereiradasilva
andrepereiradasilva - comment - 5 May 2016

@andrepereiradasilva note about bootstrap, i don't have hacked nor changed nothing in Bootstrap, just added a plugin in bootstrap.js, which extends tooltip plugin.
bootstrap.js is a one readable compiled file of all bootstrap plugins as you can customize it yourself on getbootstrap: http://getbootstrap.com/customize/#plugins
bootstrap-tooltip-extended.js is just one plugin, and nothing change in library bootstrap-tooltip.js
So here, no hacking of bootstrap, only extending. ;-)

I know. I mean changing the bootstrap(.min).js file. :smile:

What's your opinion about this other possibility ?

IMHO it would be better.

avatar JoomliC
JoomliC - comment - 5 May 2016

So, what about this in bootstrap class :

    public static function tooltipExtended($extended = true)
    {
        if ($extended)
        {
            JHtml::_('script', 'jui/bootstrap-tooltip-extended.min.js', false, true);
            JHtml::_('stylesheet', 'jui/bootstrap-tooltip-extended.css', false, true);
        }
    }

We call files with JHtml::_('bootstrap.tooltipExtended'); (this will be useful in modals)

This way, no change in bootstrap.js, and no change in core less/css files. ;-)
Note: in the demo page i've created for this plugin, the bootstrap-tooltip-extended.js is loaded separately after native bootstrap, http://www.webic.fr/bootstrap-tooltip-extended/demo-bootstrap-2.html

Should i go this way ?

avatar andrepereiradasilva
andrepereiradasilva - comment - 5 May 2016

i agree

avatar JoomliC
JoomliC - comment - 5 May 2016

@andrepereiradasilva Done! ;-)

Testing Instructions not changed: in all modals (Batch, copy Template...)
Note: no change for all tooltips where position is already set (by script, or data-placement)

avatar JoomliC JoomliC - change - 5 May 2016
The description was changed
avatar JoomliC JoomliC - change - 5 May 2016
The description was changed
avatar andrepereiradasilva andrepereiradasilva - test_item - 5 May 2016 - Tested successfully
avatar andrepereiradasilva
andrepereiradasilva - comment - 5 May 2016

I have tested this item :white_check_mark: successfully on c76be51

Nice work!


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

avatar infograf768
infograf768 - comment - 5 May 2016

will test tomorrow. :smiley:

avatar zero-24 zero-24 - test_item - 7 May 2016 - Tested successfully
avatar zero-24
zero-24 - comment - 7 May 2016

I have tested this item :white_check_mark: successfully on c76be51

It works here. Thanks. Lets give @infograf768 a chance to test too.


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

avatar brianteeman brianteeman - change - 7 May 2016
Status Pending Ready to Commit
avatar brianteeman
brianteeman - comment - 7 May 2016

rtc


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

avatar joomla-cms-bot joomla-cms-bot - change - 7 May 2016
Labels Added: ?
avatar brianteeman brianteeman - change - 7 May 2016
Milestone Added:
avatar Kubik-Rubik
Kubik-Rubik - comment - 8 May 2016

Thank you @JoomliC and testers!

avatar Kubik-Rubik Kubik-Rubik - change - 8 May 2016
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2016-05-08 13:50:15
Closed_By Kubik-Rubik
avatar Kubik-Rubik Kubik-Rubik - close - 8 May 2016
avatar Kubik-Rubik Kubik-Rubik - merge - 8 May 2016
avatar joomla-cms-bot joomla-cms-bot - close - 8 May 2016
avatar brianteeman brianteeman - close - 8 May 2016
avatar joomla-cms-bot joomla-cms-bot - change - 8 May 2016
Labels Removed: ?
avatar brianteeman brianteeman - change - 9 May 2016
Labels Added: ?

Add a Comment

Login with GitHub to post a comment