User tests: Successful: Unsuccessful:
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.
Status | New | ⇒ | Pending |
Labels |
Added:
?
|
Labels |
Added:
?
|
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.
As for the failing unit tests, I don't understand those enough to fix them. I thought I had found it, but apparently not
Labels |
Category | ⇒ | UI/UX |
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:
We should fix it in all places and I would prefer that to be done in another PR if possible.
@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.
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.
As far as I know, only Isis is using bootstrap-rtl.less
Protostar loads the compiled bootstrap-rtl.css file while Isis compiles its own template-rtl.css which includes the bootstrap-rtl.less file.
Updated to take care of RTL languages.
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
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.
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.
I have tested this item successfully on e0af65c
Isis only
I confirm:
Hathor tooltips (label) broken.
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...
Anyhow this would also require a css change to get text-align: right;
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
This PR has received new commits.
CC: @dgt41
@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.
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...
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 :)
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?
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.
Ah, haven't noticed it was a RTL screenshot, now I see
Will try later.
If you can solve then we can patch both places where popover is already used in core.
Found something which fixed the arrows for me.
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.
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 printscreen
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!
@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/
This PR has received new commits.
CC: @dgt41
That PR should still be valid.
I rebased it with current staging as it had a conflict and also squashed the commits.
I have tested this item successfully on 957eb22
TEST OK
(desktop & mobile)
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.
I have tested this item unsuccessfully on 957eb22
Not all fields with description has popover, for example in the users form
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.
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/
This PR has received new commits.
CC: @dgt41, @iljabos, @mikeveeckmans
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.
I have tested this item successfully on 009fc69
TEST OK
Hello @Bakual "This branch has conflicts that must be resolved"
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 PR has received new commits.
CC: @dgt41, @iljabos, @mikeveeckmans
@gunjanpatel @rahultailored Branch is rebased with current staging now and conflict in hathor/css/template.css is solved. Should be fine again.
Rel_Number | 0 | ⇒ | 8150 |
Relation Type | ⇒ | Related to |
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 ?
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.
I have tested this item
@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.
I checked in latest staging CMS testing successfully, now not able to see issue, Thank you for update me.
@kalpeshtailored Can you log a successful test then?
I have tested this item
Tested successfully.
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.
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).
Category | UI/UX | ⇒ | UI/UX Unit Tests |
Labels |
I have tested this item
I checked in latest staging CMS Joomla! 3.6.0-rc.
Status | Pending | ⇒ | Ready to Commit |
Labels |
Added:
?
|
@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.
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.
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 |
Labels |
Removed:
?
|
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.
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.