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

Add bootstrap tooltip extended: 4 extra positions and RTL support #10237

Merged
merged 10 commits into from May 8, 2016

Conversation

cyrezdev
Copy link
Contributor

@cyrezdev cyrezdev commented May 4, 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

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

@andrepereiradasilva
Copy link
Contributor

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.

@mbabker
Copy link
Contributor

mbabker commented May 4, 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).

@andrepereiradasilva
Copy link
Contributor

andrepereiradasilva commented May 4, 2016

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

@mbabker
Copy link
Contributor

mbabker commented May 4, 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.

@brianteeman
Copy link
Contributor

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.

@andrepereiradasilva
Copy link
Contributor

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

@mbabker
Copy link
Contributor

mbabker commented May 4, 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).

@andrepereiradasilva
Copy link
Contributor

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.

@cyrezdev
Copy link
Contributor Author

cyrezdev commented May 4, 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 ;-)

@cyrezdev
Copy link
Contributor Author

cyrezdev commented May 4, 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...! 👍
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 ?

@andrepereiradasilva
Copy link
Contributor

@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. 😄

What's your opinion about this other possibility ?

IMHO it would be better.

@SharkyKZ SharkyKZ mentioned this pull request May 5, 2016
@cyrezdev
Copy link
Contributor Author

cyrezdev commented May 5, 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 ?

@andrepereiradasilva
Copy link
Contributor

i agree

@cyrezdev
Copy link
Contributor Author

cyrezdev commented May 5, 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)

@andrepereiradasilva
Copy link
Contributor

I have tested this item ✅ successfully on c76be51

Nice work!


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

@infograf768
Copy link
Member

will test tomorrow. 😃

@zero-24
Copy link
Member

zero-24 commented May 7, 2016

I have tested this item ✅ 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.

@brianteeman
Copy link
Contributor

rtc


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label May 7, 2016
@brianteeman brianteeman added this to the Joomla 3.6.0 milestone May 7, 2016
@Kubik-Rubik
Copy link
Member

Thank you @JoomliC and testers!

@Kubik-Rubik Kubik-Rubik merged commit c600b37 into joomla:staging May 8, 2016
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label May 8, 2016
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.

None yet

8 participants