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 prettier tooltips for KPI cards #23014

Merged
merged 11 commits into from
Feb 11, 2025
Merged

Add prettier tooltips for KPI cards #23014

merged 11 commits into from
Feb 11, 2025

Conversation

caddoo
Copy link
Contributor

@caddoo caddoo commented Feb 5, 2025

Description:

Improves the tool tips for the KPI cards for all websites dashboard

(Ref: DEV-18917)

Review

@@ -137,6 +137,16 @@ describe('AllWebsitesDashboard', function () {

expect(await page.screenshotSelector('#main')).to.matchImage('dashboard_all_badges');
});

it('tooltip should show on hover of kpi card', async function() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tested one KPI card tool tip, the logic is reused, I don't want to add too much additional time to the UI test suite.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note here @caddoo, the method it reads nicely when followed by wording that creates a sentence, e.g. here it would be it('should show a tooltip on hover of KPI card').

@caddoo caddoo marked this pull request as ready for review February 5, 2025 02:58
@caddoo caddoo requested a review from a team February 5, 2025 02:58
@caddoo caddoo added the Needs Review PRs that need a code review label Feb 5, 2025
@caddoo caddoo requested a review from sgiehl February 6, 2025 23:26
@sgiehl
Copy link
Member

sgiehl commented Feb 10, 2025

@caddoo Actually it seems like the tooltips are now not working anymore at all for KPIs. Not sure if that is a bug or an issue in the tooltip directive. But I thought it should also work without a title attribute 🤔

@caddoo
Copy link
Contributor Author

caddoo commented Feb 11, 2025

@caddoo Actually it seems like the tooltips are now not working anymore at all for KPIs. Not sure if that is a bug or an issue in the tooltip directive. But I thought it should also work without a title attribute 🤔

@sgiehl

Yeah I noticed that as well during development, I mistakenly thought it started working again while adding it back.

Anyway I looked for a few minutes and couldn't work it out, I don't think I want to spend time looking into jQuery UI tool tips to find out what causes this behavior.

But also your concern about the title showing in the browser native way won't be an issue, if you look at the DOM when the jQuery UI tooltip opens it clears the title attribute anyway 🤷 .

I suppose for the sake of time it's fine just to add the title and move on from this.

@sgiehl
Copy link
Member

sgiehl commented Feb 11, 2025

@caddoo Just for completeness: The behavior is quite easy explained. The jQuery tooltips in the directive are initialized without an items option. That means that it will look for any element (within scope) that has a title attribute and initialize a tooltip for it with the other provided options. To make it work without a title attribute and for a specific element only the tooltip directive would need to be adjusted to allow passing in the items option.

@sgiehl sgiehl added this to the 5.3.0 milestone Feb 11, 2025
@sgiehl sgiehl added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Feb 11, 2025
@sgiehl sgiehl merged commit 9b3f54c into 5.x-dev Feb 11, 2025
22 of 26 checks passed
@sgiehl sgiehl deleted the dev-18918 branch February 11, 2025 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants