? ? Success
Related to # 8150

User tests: Successful: Unsuccessful:

avatar Bakual
Bakual
27 Oct 2015

Inspired by #8150
This PR uses the Bootstrap "popover" method to display a tooltip when there is a title and description present in a form.
To test, you can have a look at an article edit form. The "Title" field doesn't have a description, thus a regular tooltip is shown. The alias does have both, thus the popover will be used.

In backend this worked fine for me. In frontend, the placement was looking bad because it goes that far right. Not sure why it behaves different there.
I tried with position it on top, but then it got cut in backend for the alias, so that isn't ideal either. And I think it actually looks better when placed right there.

Votes

# of Users Experiencing Issue
1/1
Average Importance Score
4.00

avatar Bakual Bakual - open - 27 Oct 2015
avatar Bakual Bakual - change - 27 Oct 2015
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 27 Oct 2015
Labels Added: ?
avatar Bakual Bakual - change - 27 Oct 2015
The description was changed
avatar joomla-cms-bot joomla-cms-bot - change - 27 Oct 2015
Labels Added: ?
avatar infograf768
infograf768 - comment - 27 Oct 2015

NIce and simple but works only for labels, not the other tooltips. Indeed an issue in frontend where not only does it show far away from the label (defeating the purpose of the tip and totally invisible on small screens, see picture) but also one can have multiple type of tips on the same page:
Edit an article: "Category" will show the old way and the other tips the new way.

screen shot 2015-10-27 at 11 15 40

Note: concerning #8150, it needs more care as it also concerns Hathor, Protostar and Beez.
Please let me know if I go on working on it or not.

avatar Bakual
Bakual - comment - 27 Oct 2015

We had the same alignment issue with our tooltips, which was fixed with #5137. I have now included the same fix for popovers as well.
Alignment should be fine now.

And yes, it only affects forms as this is the place where I expected a title/content combination. This should be B/C as long as the template supports popovers, which a good template should support anyway.
Other places would have to be fixed in the extension itself.

avatar Bakual
Bakual - comment - 27 Oct 2015

As for the failing unit tests, I don't understand those enough to fix them. I thought I had found it, but apparently not :blush:

avatar zero-24 zero-24 - change - 27 Oct 2015
Labels
avatar zero-24 zero-24 - change - 27 Oct 2015
Category UI/UX
avatar infograf768
infograf768 - comment - 27 Oct 2015

@Bakual
This is not RTL aware: text direction as well as tip placement.

avatar Bakual
Bakual - comment - 27 Oct 2015

This is not RTL aware: text direction as well as tip placement.

Good find. That seems to be unrelated to this PR. I have found two places in core where we already use popovers and it doesn't work nice with RTL there as well:

  • Smart Search main view (index). If you hover over the calendar icon.
  • Module Manager when you create a new module, hover over the description text of the module.

We should fix it in all places and I would prefer that to be done in another PR if possible.

avatar infograf768
infograf768 - comment - 27 Oct 2015

@Bakual
I have a solution for rtl (if this PR is desired). modified code based on your PR.
This for Isis, but same can be done for the other core templates.

diff --git a/layouts/joomla/form/renderlabel.php b/layouts/joomla/form/renderlabel.php
index 0520c9f..48f3079 100644
--- a/layouts/joomla/form/renderlabel.php
+++ b/layouts/joomla/form/renderlabel.php
@@ -34,7 +34,24 @@
 if (!empty($desc))
 {
-   JHtml::_('bootstrap.tooltip');
-   $classes[] = 'hasTooltip';
-   $title     = ' title="' . JHtml::tooltipText(trim($text, ':'), $desc, 0) . '"';
+   if ($text && $text != $desc)
+   {
+       JHtml::_('bootstrap.popover');
+       $classes[] = 'hasPopover';
+       $title     = ' title="' . htmlspecialchars(trim($text, ':')) . '"'
+           . ' data-content="'. htmlspecialchars($desc) . '"';
+
+       if (JFactory::getLanguage()->isRtl())
+       {
+           if ($position == '')
+           {
+               $position = ' data-placement="left" ';
+           }
+       }
+   }
+   else
+   {
+       JHtml::_('bootstrap.tooltip');
+       $classes[] = 'hasTooltip';
+       $title     = ' title="' . JHtml::tooltipText(trim($text, ':'), $desc, 0) . '"';
+   }
 }
diff --git a/administrator/templates/isis/less/template-rtl.less b/administrator/templates/isis/less/template-rtl.less
index 2dd8f68..8e081de 100644
--- a/administrator/templates/isis/less/template-rtl.less
+++ b/administrator/templates/isis/less/template-rtl.less
@@ -247,2 +247,7 @@
    }
 }
+
+/* Correcting popover text-direction */
+.popover {
+   text-align: right;
+}
\ No newline at end of file
diff --git a/administrator/templates/isis/css/template-rtl.css b/administrator/templates/isis/css/template-rtl.css
index 3b7bfbb..55f7a7e 100644
--- a/administrator/templates/isis/css/template-rtl.css
+++ b/administrator/templates/isis/css/template-rtl.css
@@ -1672,4 +1672,5 @@
    padding-left: 180px;
 }
+.control-label .hasPopover,
 .control-label .hasTooltip {
    display: inline-block;
@@ -8925,2 +8926,5 @@
    }
 }
+.popover {
+   text-align: right;
+}

Note: as stated in the other PR, this needs CSS changes in every template. So I wonder about B/C.

avatar infograf768
infograf768 - comment - 27 Oct 2015

We will get:
screen shot 2015-10-27 at 15 38 30

avatar Bakual
Bakual - comment - 27 Oct 2015

I think we could add the RTL CSS rule to this (tooltip) block: https://github.com/joomla/joomla-cms/blob/staging/media/jui/less/bootstrap-rtl.less#L582-L585
Looks like it's where we fixed the tooltips, so any templates which are built using that LESS file should get the fix.

The data-placement will work for most cases I think. What I like about the default value is that I think it automatically changes the placement if there is no room to show it. Like if there is no room on the right, it will go down or so. But have to test that.

avatar infograf768
infograf768 - comment - 27 Oct 2015

As far as I know, only Isis is using bootstrap-rtl.less

avatar Bakual
Bakual - comment - 27 Oct 2015

Protostar loads the compiled bootstrap-rtl.css file while Isis compiles its own template-rtl.css which includes the bootstrap-rtl.less file.

avatar Bakual
Bakual - comment - 27 Oct 2015

Updated to take care of RTL languages.

avatar Bakual
Bakual - comment - 27 Oct 2015

Popovers don't look nice for Hathor, but they are already used in the modul manager. So that would have to be fixed anyway.
Same for Beez, doesn't look nice. That's a shortcoming of this template and will also be true for any 3rd party extension that uses Popovers. It's a shame we ship core templates which don't support all features the core has. If someone wants to propose supporting CSS, I'm all ears :smile:

avatar mbabker
mbabker - comment - 27 Oct 2015

The problem with both of those templates is they aren't natively Bootstrap designed. In the case of Hathor specifically it has bits and pieces of Bootstrap hacked into it but the template was never fully Bootstrap capable.

avatar Bakual
Bakual - comment - 27 Oct 2015

Yeah, I know that. And thus it fails on any Bootstrap feature we actually have APIs within the CMS to use. It's a bad example we do here.

avatar dgt41 dgt41 - test_item - 27 Oct 2015 - Tested successfully
avatar dgt41
dgt41 - comment - 27 Oct 2015

I have tested this item :white_check_mark: successfully on e0af65c

Isis only


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

avatar infograf768
infograf768 - comment - 28 Oct 2015

I confirm:

Hathor tooltips (label) broken.
screen shot 2015-10-28 at 08 40 21

Beez tooltips broken
screen shot 2015-10-28 at 08 45 10

I first thought the solution could have been to create a specific layout for these 2 templates so that popovers are not implemented but it may be a better idea to do the opposite, i.e. not change the core layout but, instead, add the patched layout in Isis and Protostar. The advantage of that is that it would be totally B/C.
What do you think?

Concerning the Modules Select page, we have a bug in RTL.
it should be:

<?php else : ?>
    <li>
        <small class="hasPopover" data-placement="left" 

instead of

<?php else : ?>
    <li>
        <small rel="popover" data-placement="left"

I will make a specific PR as this concerns present staging.
EDIT: Hmm, the popover disapears when we reduce the window size.
I tried to use instead using data-placement="top" there but I get a weird arrow...
screen shot 2015-10-28 at 09 41 49

Anyhow this would also require a css change to get text-align: right;

avatar Bakual
Bakual - comment - 28 Oct 2015

We could do a template override just for Isis and Protostar and be totally B/C.
On the other hand, the templates which will have issues with popovers, already have those issues with any extension which uses them.
Since popovers are actually a feature of our core (we have an API to create them!) I'm more favoured in changing the core output.
Yes it may affect templates which aren't supporting everything they should (read: bad templates), but I don't think it's that big of an issue. It will only affect tooltips of a form.
Imho, it would even be a good thing as it forces templates to support popovers better.
We may have to add basic instructions to our doc pages with basic CSS to support popover (and tooltips). Basically include the rules from popover.less :smile:

avatar joomla-cms-bot
joomla-cms-bot - comment - 28 Oct 2015

This PR has received new commits.

CC: @dgt41


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

avatar Bakual
Bakual - comment - 28 Oct 2015

@infograf768 Works now also for Isis and Hathor. As a side effect, the "New Module" view in the Hathor module manager now has working popovers as well.

I leave it to others to decide if we want to do that only for our core templates (using JLayout overrides) or change it in core.

avatar infograf768
infograf768 - comment - 28 Oct 2015

What about the Modules Select page? I did not make a PR because

EDIT: Hmm, the popover disapears when we reduce the window size.
I tried to use instead using data-placement="top" there but I get a weird arrow...

avatar Bakual
Bakual - comment - 28 Oct 2015

That would be an exsting bug anyway since that view already uses popovers in Isis. The same happens there as well. I'm not sure if it needs to be fixed though. It's not like it's that important :)

avatar infograf768
infograf768 - comment - 28 Oct 2015

The issue is clearly the use of data-placement="top" or "bottom" which makes bootstrap popover arrow failing.
Is it a known bug ? Can we solve it ourselves?

avatar Bakual
Bakual - comment - 28 Oct 2015

Hmm, looks fine in Chrome and IE for me when trying both with placement top and bottom. Looks like a bug in your browser then?
How does the popver look for the Alias field in the article edit form? That one would be placed on bottom as well.

avatar infograf768
infograf768 - comment - 28 Oct 2015

the arrow issue happens only in RTL.
screen shot 2015-10-28 at 16 11 36

avatar Bakual
Bakual - comment - 28 Oct 2015

Ah, haven't noticed it was a RTL screenshot, now I see :smile:
Will try later.

avatar infograf768
infograf768 - comment - 28 Oct 2015

If you can solve then we can patch both places where popover is already used in core.

avatar Bakual
Bakual - comment - 28 Oct 2015

Found something which fixed the arrows for me.

avatar infograf768
infograf768 - comment - 29 Oct 2015

I had found another solution, but that one works too (in Isis) and is simpler than mine.
Hathor is fine when colour chosen is not "High Contrast" (Who still uses that?...).

These should be added to the frontend templates I guess if it is decided to put the layout in core.

You now can correct the Select module by setting the data-placement to Top which avoids disapearing tip in LTR or RTL when reducing the window and solves also the rtl missing class in the former code:

diff --git a/administrator/components/com_modules/views/select/tmpl/default.php b/administrator/components/com_modules/views/select/tmpl/default.php
index 60ca2fa..2ac5ce7 100644
--- a/administrator/components/com_modules/views/select/tmpl/default.php
+++ b/administrator/components/com_modules/views/select/tmpl/default.php
@@ -25,19 +25,11 @@
    <?php $short_desc = JHtml::_('string.truncate', ($this->escape(strip_tags($item->desc))), 90); ?>

-   <?php if ($document->direction != "rtl") : ?>
    <li>
        <a href="<?php echo JRoute::_($link);?>">
            <strong><?php echo $name; ?></strong>
        </a>
-       <small class="hasPopover" data-placement="right" title="<?php echo $name; ?>" data-content="<?php echo $desc; ?>"><?php echo $short_desc; ?></small>
+       <small class="hasPopover" data-placement="top" title="<?php echo $name; ?>" data-content="<?php echo $desc; ?>"><?php echo $short_desc; ?></small>
    </li>
-   <?php else : ?>
-   <li>
-       <small rel="popover" data-placement="left" title="<?php echo $name; ?>" data-content="<?php echo $desc; ?>"><?php echo $short_desc; ?></small>
-       <a href="<?php echo JRoute::_($link); ?>">
-           <strong><?php echo $name; ?></strong>
-       </a>
-   </li>
-   <?php endif; ?>
+
 <?php endforeach; ?>
 </ul>

I guess that anyway, even if it is not decided to put the layout in core but only for our templates, the css corrections have to be done and the select modules patched.

avatar mqueme
mqueme - comment - 7 Nov 2015

Tested this on the mobile version, using the articles alias example you provided. When clicking in a specific layout it shows fine, but when you rotate device the pop up doesn't move to the correct position of the label. Providing a printscreenscreen shot 2015-11-07 at 04 43 23


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

avatar roland-d
roland-d - comment - 14 Apr 2016

Hello @Bakual

Thank you for your contribution.

The last comment here was on 7th November 2015. So the question is, Is this issue/pull request still valid?
If no reply is received within 4 weeks we will close this issue.

Thanks for understanding!


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

avatar brianteeman
brianteeman - comment - 14 Apr 2016

@rolandd it is our fault that there has been no activity not the creator of
the pull request

On 14 April 2016 at 17:08, RolandD notifications@github.com wrote:

Hello @Bakual https://github.com/Bakual

Thank you for your contribution.

The last comment here was on 7th November 2015. So the question is, Is
this issue/pull request still valid?
If no reply is received within 4 weeks we will close this issue.

Thanks for understanding!

This comment was created with the J!Tracker Application
https://github.com/joomla/jissues at issues.joomla.org/joomla-cms/8174
https://issues.joomla.org/tracker/joomla-cms/8174.


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#8174 (comment)

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

avatar joomla-cms-bot
joomla-cms-bot - comment - 14 Apr 2016

This PR has received new commits.

CC: @dgt41


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

avatar Bakual
Bakual - comment - 14 Apr 2016

That PR should still be valid.
I rebased it with current staging as it had a conflict and also squashed the commits.

avatar mikeveeckmans mikeveeckmans - test_item - 15 Apr 2016 - Tested successfully
avatar mikeveeckmans
mikeveeckmans - comment - 15 Apr 2016

I have tested this item :white_check_mark: successfully on 957eb22

TEST OK
(desktop & mobile)


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

avatar brianteeman
brianteeman - comment - 15 Apr 2016

I am not sure if this is correct. I have one tooltip with the popover and one with the old tooltip ut they both look to me as if they have a title and description.

screen shot 2016-04-15 at 06 36 34

screen shot 2016-04-15 at 06 37 01


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

avatar iljabos iljabos - test_item - 15 Apr 2016 - Tested unsuccessfully
avatar iljabos
iljabos - comment - 15 Apr 2016

I have tested this item :red_circle: unsuccessfully on 957eb22

Not all fields with description has popover, for example in the users form


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

avatar iljabos
iljabos - comment - 15 Apr 2016

screen shot 2016-04-15 at 07 26 41


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

avatar Bakual
Bakual - comment - 15 Apr 2016

The table headers("select to sort by this column") aren't generated by JForm and thus they aren't changed. That's expected so far but good find.
I think those could be changed with another PR if this one gets accepted. Shouldn't be to hard to do.

avatar brianteeman
brianteeman - comment - 15 Apr 2016

Whatever happens i think we must be consistent

On 15 April 2016 at 14:51, Thomas Hunziker notifications@github.com wrote:

The table headers("select to sort by this column") aren't generated by
JForm and thus they aren't changed. That's expected so far but good find.
I think those could be changed with another PR if this one gets accepted.
Shouldn't be to hard to do.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#8174 (comment)

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

avatar joomla-cms-bot
joomla-cms-bot - comment - 15 Apr 2016

This PR has received new commits.

CC: @dgt41, @iljabos, @mikeveeckmans


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

avatar Bakual
Bakual - comment - 15 Apr 2016

Added popovers for the table headers as well.
Keep in mind when testing to also check frontend. Also the table headers use different methods depending if searchtools or the old filters (eg in Smart Search) are used, so please test both.

avatar mikeveeckmans mikeveeckmans - test_item - 18 Apr 2016 - Tested successfully
avatar mikeveeckmans
mikeveeckmans - comment - 18 Apr 2016

I have tested this item :white_check_mark: successfully on 009fc69

TEST OK


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

avatar gunjanpatel
gunjanpatel - comment - 1 Jul 2016

Hello @Bakual "This branch has conflicts that must be resolved"


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

avatar rahulkoshti
rahulkoshti - comment - 1 Jul 2016

Hello @Bakual,

I have tried to apply patch but it doesn't successful. I think it is because Merge Conflicts like @gunjanpatel said.

Thanks.


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

avatar joomla-cms-bot
joomla-cms-bot - comment - 1 Jul 2016

This PR has received new commits.

CC: @dgt41, @iljabos, @mikeveeckmans


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

avatar Bakual
Bakual - comment - 1 Jul 2016

@gunjanpatel @rahultailored Branch is rebased with current staging now and conflict in hathor/css/template.css is solved. Should be fine again.

avatar gunjanpatel gunjanpatel - change - 1 Jul 2016
Rel_Number 0 8150
Relation Type Related to
avatar kalpesh681
kalpesh681 - comment - 1 Jul 2016

I have checked Article edit pages in mobile view and desktop view, and found only category field tool-tip and another fields tool-tips just want to know that, Is it an issue ?

screen shot 2016-07-01 at 07 57 05screen shot 2016-07-01 at 07 57 11


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

avatar Bakual
Bakual - comment - 1 Jul 2016

The "Category" field tooltip doesn't has a title, just the "category" text (quite useless actually). Thus it shows as a regular tooltip (white font on black ground).
The "Tags" field has a title and a description as the tooltip and thus it uses the new popover (black font on white ground).

The other fields below (Version Note, Author's Alias, Status, Featured, ...) all also should have a "popover"-style tooltip.

avatar kalpesh681 kalpesh681 - test_item - 1 Jul 2016 - Tested unsuccessfully
avatar kalpesh681
kalpesh681 - comment - 1 Jul 2016

I have tested this item ? unsuccessfully on bfda664


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

avatar Bakual
Bakual - comment - 1 Jul 2016

@kalpesh681 Can you be more specific about your enviroment? Because when I try to reproduce the issue with Chrome (faking a Galaxy S5) it seems to work fine. I also don't see a reason why it shouldn't work for specifc fields. The code is always the same.

avatar kalpeshtailored
kalpeshtailored - comment - 4 Jul 2016

@Bakual I checked in Firefox 47.0 and screen size 320 X480 (faking an IPhone 4) size.

avatar kalpeshtailored
kalpeshtailored - comment - 8 Jul 2016

I checked in latest staging CMS testing successfully, now not able to see issue, Thank you for update me.


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

avatar gunjanpatel gunjanpatel - alter_testresult - 8 Jul 2016 - kalpesh681: Tested successfully
avatar gunjanpatel gunjanpatel - alter_testresult - 8 Jul 2016 - kalpeshtailored: Tested successfully
avatar gunjanpatel gunjanpatel - alter_testresult - 8 Jul 2016 - kalpesh681: Not tested
avatar Bakual
Bakual - comment - 8 Jul 2016

@kalpeshtailored Can you log a successful test then?

avatar kalpeshtailored kalpeshtailored - test_item - 11 Jul 2016 - Tested successfully
avatar kalpeshtailored
kalpeshtailored - comment - 11 Jul 2016

I have tested this item successfully on bfda664

Tested successfully.


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

avatar hardiktailored
hardiktailored - comment - 12 Jul 2016

I found z-index issue with popover while editing article. Check the "Tags" option in image below. I also notice that border cuts down in smaller screens but I think real device may not have same issue because I am testing using Chrome(Version 48.0.2564.103 (64-bit)) mobile testing tool.
screen shot 2016-07-12 at 02 14 09


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

avatar Bakual
Bakual - comment - 12 Jul 2016

I don't think the z-index issue is something that has to be fixed. Select options most likely should stay on top anyway.
Anyway, this would be unrelated to this PR as it would be a general issue with popovers (and most likely tooltips as well).

avatar brianteeman brianteeman - change - 12 Jul 2016
Category UI/UX UI/UX Unit Tests
avatar brianteeman brianteeman - change - 12 Jul 2016
Labels
avatar rahultailored rahultailored - test_item - 12 Jul 2016 - Tested successfully
avatar rahultailored
rahultailored - comment - 12 Jul 2016

I have tested this item successfully on bfda664

I checked in latest staging CMS Joomla! 3.6.0-rc.


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

avatar gunjanpatel gunjanpatel - change - 13 Jul 2016
Status Pending Ready to Commit
avatar roland-d
roland-d - comment - 16 Jul 2016

@Bakual I was testing this issue and have a question. Can the search field of the Search Tools have a popover as well?

screen shot 2016-07-16 at 07 33 28

avatar joomla-cms-bot joomla-cms-bot - change - 16 Jul 2016
Labels Added: ?
avatar Bakual
Bakual - comment - 16 Jul 2016

@roland-d Sure, We just would have to change this line: https://github.com/joomla/joomla-cms/blob/staging/layouts/joomla/searchtools/default/bar.php#L39 to call popover instead of tooltip.
And in https://github.com/joomla/joomla-cms/blob/staging/layouts/joomla/searchtools/default/bar.php#L39 remove the tooltipText call and change it to directly insert title and data-content attributes.

avatar roland-d
roland-d - comment - 16 Jul 2016

@Bakual Do you want to do that in this PR or should we get a separate one for that?

avatar Bakual
Bakual - comment - 16 Jul 2016

I'd rather do it in an own. This one is for the places where there is a title and description present. The searchfield only has one of it so it would be something else.

avatar roland-d roland-d - change - 16 Jul 2016
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2016-07-16 17:46:39
Closed_By roland-d
avatar roland-d
roland-d - comment - 16 Jul 2016

@Bakual Fair enough, merged this one :)

avatar joomla-cms-bot joomla-cms-bot - change - 16 Jul 2016
Labels Removed: ?

Add a Comment

Login with GitHub to post a comment