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

Use improved tooltips for all tooltips in the visitor log #8545

Merged
merged 2 commits into from Aug 17, 2015

Conversation

Projects
None yet
2 participants
@mnapoli
Contributor

mnapoli commented Aug 11, 2015

Fixes #8356

Example (not limited to this tooltip):

capture d ecran 2015-08-11 a 13 20 30

@tsteur

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Aug 11, 2015

Member

I kinda started working on this issue at the same time but only for like 20 minutes. I wanted to do it for all elements containing a [title] attribute (excluding the elements that already have tooltips because they have custom formatting I think) automatically so it will also work in the future. Just saying in case this would be an option as well.

Member

tsteur commented Aug 11, 2015

I kinda started working on this issue at the same time but only for like 20 minutes. I wanted to do it for all elements containing a [title] attribute (excluding the elements that already have tooltips because they have custom formatting I think) automatically so it will also work in the future. Just saying in case this would be an option as well.

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Aug 11, 2015

Contributor

@tsteur that was my first thought too, but the amount of things that could (or could not ofc) break (or go wrong) left me choosing the most secure solution. There is already a lot of custom code for showing tooltips here and there, that would be great at some point to remove it.

Contributor

mnapoli commented Aug 11, 2015

@tsteur that was my first thought too, but the amount of things that could (or could not ofc) break (or go wrong) left me choosing the most secure solution. There is already a lot of custom code for showing tooltips here and there, that would be great at some point to remove it.

<li>
{{ 'General_Plugins'|translate }}:
{% for pluginIcon in visitor.getColumn('pluginsIcons') %}
<img src="{{ pluginIcon.pluginIcon }}" alt="{{ pluginIcon.pluginName|capitalize(true) }}"/>

This comment has been minimized.

@tsteur

tsteur Aug 13, 2015

Member

is there a title attribute missing now? maybe on purpose?

@tsteur

tsteur Aug 13, 2015

Member

is there a title attribute missing now? maybe on purpose?

This comment has been minimized.

@mnapoli

mnapoli Aug 13, 2015

Contributor

On purpose, there is already the alt for screen readers. And since it's in a tooltip, it's impossible to hover (and then show another tooltip in a tooltip), so I've removed it.

@mnapoli

mnapoli Aug 13, 2015

Contributor

On purpose, there is already the alt for screen readers. And since it's in a tooltip, it's impossible to hover (and then show another tooltip in a tooltip), so I've removed it.

@tsteur

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Aug 14, 2015

Member

I'm not sure if I'm doing something wrong but it doesn't seem to work for me. I made sure to checkout right branch, clear templates cache and that correct file is loaded. The date, ip, custom variables title still shows the plain tooltip for me in Chrome 44 on Mac.

Member

tsteur commented Aug 14, 2015

I'm not sure if I'm doing something wrong but it doesn't seem to work for me. I made sure to checkout right branch, clear templates cache and that correct file is loaded. The date, ip, custom variables title still shows the plain tooltip for me in Chrome 44 on Mac.

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Aug 14, 2015

Contributor

I checked again, I'm in sync with origin, I checked out the branch, cleared the cache, and it works.

capture d ecran 2015-08-14 a 14 27 40

Can someone else give it a try?

Contributor

mnapoli commented Aug 14, 2015

I checked again, I'm in sync with origin, I checked out the branch, cleared the cache, and it works.

capture d ecran 2015-08-14 a 14 27 40

Can someone else give it a try?

@tsteur

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Aug 14, 2015

Member

Does it work when hovering eg the date?

Member

tsteur commented Aug 14, 2015

Does it work when hovering eg the date?

tsteur added a commit that referenced this pull request Aug 17, 2015

Merge pull request #8545 from piwik/8356
Use improved tooltips for all tooltips in the visitor log

@tsteur tsteur merged commit f8e3d2d into master Aug 17, 2015

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Scrutinizer No new issues
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@tsteur tsteur deleted the 8356 branch Aug 17, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment