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

New tooltip replacing #6050 #8150

Closed
wants to merge 5 commits into from
Closed

Conversation

infograf768
Copy link
Member

@infograf768 infograf768 commented Oct 25, 2015

See #6050

This solves the <strong> issue.

After patch (clear your caches) you should get:
screen shot 2015-10-25 at 09 31 25
screen shot 2015-10-25 at 10 16 01
screen shot 2015-10-25 at 10 16 28

@zero-24
Copy link
Contributor

zero-24 commented Oct 25, 2015

Travis is failing here :( maybe because of different html?

@roland-d
Copy link
Contributor

@zero-24 Indeed, the unit tests fail due to the changed HTML. The unit test should be updated as well to reflect the change in HTML.

@infograf768
Copy link
Member Author

I am trying to do that now.

@ghost
Copy link

ghost commented Oct 25, 2015

@test Successfully tested with ISIS and current staging. ✅

@joomla-cms-bot
Copy link

This PR has received new commits.

CC: @bertmert


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

@infograf768
Copy link
Member Author

CC: @adhocgraFX, @christianhent, @designbengel, @KingLouis1, @RoterNagel, @Yorgoz
Please test.

@fontanil
Copy link

@test
Patch is OK. Thanks!

@joomla-cms-bot
Copy link

This PR has received new commits.

CC: @bertmert, @fontanil


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

@infograf768
Copy link
Member Author

@bertmert
Although we do not need to escape single quotes when using JText it does not cost much to do so in this precise context. Modified PR.

@infograf768
Copy link
Member Author

@bertmert, @fontanil
Please retest and confirm on issues.joomla.org + test also
#8161

thanks

@fontanil
Copy link

@test
I confirm. Test OK


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

@ghost
Copy link

ghost commented Oct 26, 2015

@test Tested successfully ✅ this PR in combination with #8161

@zero-24 zero-24 added this to the Joomla! 3.5.0 milestone Oct 26, 2015
@zero-24
Copy link
Contributor

zero-24 commented Oct 26, 2015

RTC for 3.5.0. Thanks


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Oct 26, 2015
@dgrammatiko
Copy link
Contributor

@infograf768 I like it a lot but I think if we go for a light colored tool tip we should apply some shadow around it (same as modal, popup, menus)
So here is a preview of that:
screenshot 2015-10-26 16 17 19
And the changes:
jui/tooltip.less after line 5

// Base class
.tooltip {
  position: absolute;
  z-index: @zindexTooltip;
  display: block;
  visibility: visible;
  font-size: 11px;
  line-height: 1.4;
  .opacity(0);
  .border-radius(@baseBorderRadius);
  &.in     { .opacity(80); }
  &.top    { margin-top:  -3px; padding: 0px 0; }
  &.right  { margin-left:  3px; padding: 0 5px; }
  &.bottom { margin-top:   3px; padding: 0px 0; }
  &.left   { margin-left: -3px; padding: 0 5px; }
}

and isis/template.less after line 1048

  .box-shadow(0 0 5px rgba(0,0,0,0.3));

Same thing for protostar

@roland-d
Copy link
Contributor

@infograf768 Would you like to add @dgt41 suggestion to this PR?

@infograf768
Copy link
Member Author

@dgt41

Pleaase contact me on glip


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

@infograf768
Copy link
Member Author

@roland-d
I tested @dgt41 suggestion and It's missing something here.
Concerning Protostar, I suggest to do a separate PR

@roland-d
Copy link
Contributor

@infograf768 Fine by me, then I will merge this as-is.

@infograf768
Copy link
Member Author

@roland-d
Please wait I talked to @dgt41

@Bakual
Copy link
Contributor

Bakual commented Oct 27, 2015

You guys are aware that Bootstrap already has a thing called "Popover" which would look exactly like that? See http://getbootstrap.com/2.3.2/javascript.html#popovers
Bootstrap tooltips aren't meant to contain a title or lengthy texts, that's what popver are designed for. The JS is the same, just a difference in appearance.

@infograf768
Copy link
Member Author

Would not changing from tooltip to popovers be much more complex and maybe not B/C?

@infograf768
Copy link
Member Author

@dgt41
After some modifications to your proposal, I can obtain this:
screen shot 2015-10-27 at 09 28 00

@Bakual
If you have a better idea, please propose some code.

@Bakual
Copy link
Contributor

Bakual commented Oct 27, 2015

See #8174 for an idea how to use Popovers. You will see that they look about the same to what you try to do here.
So you basically remove tooltips from backend and make everything a popover.

The main issue is however that we did misuse tooltips back when we started using Bootstrap tooltips. They were never meant to show titles to begin with. We hacked around it instead of using the proper method 😄

@infograf768
Copy link
Member Author

Folks, I succeeded in patching also Hathor, Beez and Protostar, but, doing so, I found out that this PR is not B/C at all for the other templates around because of the change in the library.
I.e. any template not updating their css will not get any more the tooltip title in bold and will keep the background colour and text colour the template maker has set.

I guess therefore that it is not the solution. Commented on @Bakual suggestion in #8174 (comment)

@zero-24
Copy link
Contributor

zero-24 commented Oct 27, 2015

Ok so for now we remove the milestone and label.


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

@joomla-cms-bot joomla-cms-bot removed this from the Joomla! 3.5.0 milestone Oct 27, 2015
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Oct 27, 2015
@coolcat-creations
Copy link
Contributor

I have tested this item ✅ successfully on 142ce1e


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

@joomla-cms-bot
Copy link

This PR has received new commits.

CC: @bertmert, @designbengel, @fontanil


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

@infograf768
Copy link
Member Author

In case of, I also took care of Hathor, Beez and Protostar.
To test with English and an RTL language.

@ghost
Copy link

ghost commented Nov 13, 2015

Even if I don't know if this PR is still relevant.
@test Tested successfully with all core templates BE and FE

Concerning BC and STRONG. The main conflict in the beginning was that STRONG was formatted as block. This has been removed now and .tooltip-title is block now.

So, I think changing line https://github.com/joomla/joomla-cms/pull/8150/files#diff-b01d47e3fbe4a8e5fb4987f4df64b291R920

to

$result = '<span class="tooltip-title"><strong>' . $title . '</strong></span>';

or

$result = '<span class="tooltip-title"><b>' . $title . '</b></span>';

could be a solution for BC

@ghost
Copy link

ghost commented Nov 14, 2015

Found another issue with Hathor (not directly related to the changes of this patch but also same behavior with this patch).
#8434

@brianteeman
Copy link
Contributor

@infograf768 I'm a bit confused on the status of this PR


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

@brianteeman
Copy link
Contributor

I guess this PR is no longer needed.


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

@brianteeman brianteeman closed this May 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants