Skip to content

Fix tooltip content logic in bar_chart_goals function to handle null values and improve readability.#44

Merged
Touxten merged 2 commits intoPrestaShop:devfrom
cnavarro-prestashop:fix/graph-undefined
Apr 2, 2026
Merged

Fix tooltip content logic in bar_chart_goals function to handle null values and improve readability.#44
Touxten merged 2 commits intoPrestaShop:devfrom
cnavarro-prestashop:fix/graph-undefined

Conversation

@cnavarro-prestashop
Copy link
Copy Markdown
Contributor

Questions Answers
Description? Fix tooltip content logic in bar_chart_goals function to handle null values and improve readability.
Type? bug fix
BC breaks? no
Deprecations? no
Fixed ticket? -
How to test? Display dash forecast

@cnavarro-prestashop cnavarro-prestashop requested a review from a team March 27, 2026 17:58
@ps-jarvis
Copy link
Copy Markdown

Hello @cnavarro-prestashop!

This is your first pull request on dashgoals repository of the PrestaShop project.

Thank you, and welcome to this Open Source community!

@github-project-automation github-project-automation Bot moved this to Ready for review in PR Dashboard Mar 27, 2026
jolelievre
jolelievre previously approved these changes Mar 30, 2026
@ps-jarvis ps-jarvis added the Waiting for QA Status: Action required, Waiting for test feedback label Mar 30, 2026
@ps-jarvis ps-jarvis moved this from Ready for review to To be tested in PR Dashboard Mar 30, 2026
mattgoud
mattgoud previously approved these changes Mar 30, 2026
@SiraDIOP SiraDIOP self-assigned this Mar 30, 2026
@Progi1984 Progi1984 added this to the 2.0.5 milestone Mar 30, 2026
Copy link
Copy Markdown

@SiraDIOP SiraDIOP left a comment

Choose a reason for hiding this comment

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

Hello @cnavarro-prestashop ,

I tested this PR on PS 9.2.0 with Docker.

✅ No regression observed
✅ No undefined displayed on bar hover
✅ Forecast widget loads correctly with real data

⚠️ However, the tooltip does not appear at all. The browser console shows the following warning:

nvd3 warning: `tooltipContent` has been deprecated. use chart.tooltip.contentGenerator() instead

It seems the tooltipContent method is deprecated in the version of nvd3 used by PS 9.x, which prevents the tooltip from rendering entirely.

The null value fix looks good, but it might be worth migrating to chart.tooltip.contentGenerator() to ensure the tooltip actually displays on PS 9.

Enregistrement.de.l.ecran.2026-03-30.a.17.01.04.mov
Enregistrement.de.l.ecran.2026-03-30.a.17.15.11.mov

Did i miss something?

@cnavarro-prestashop cnavarro-prestashop dismissed stale reviews from mattgoud and jolelievre via 18aa5d9 April 1, 2026 16:27
@cnavarro-prestashop
Copy link
Copy Markdown
Contributor Author

Hi @SiraDIOP,

Thanks for the test. I made modification you suggested about replacing method NVD3 tooltipContent by tooltip.contentGenerator.

After checking with some test it seems Warning messages have completely disappeared from the console and now I've well the tooltip displayed :

Enregistrement.de.l.ecran.2026-04-01.a.18.24.02.mov

Copy link
Copy Markdown

@SiraDIOP SiraDIOP left a comment

Choose a reason for hiding this comment

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

Hello @cnavarro-prestashop
Tested this PR on PS 9.1.0 with a fresh install of the dashgoals module. The tooltip displays correctly when hovering over the bars in the Forecast widget — it shows the sales value and the goal set as expected. ✅

Enregistrement.de.l.ecran.2026-04-02.a.17.08.31.mov

One thing noticed: when uninstalling and reinstalling the module, the Forecast widget moves to the bottom of the dashboard instead of staying in its original position. This seems unrelated to this PR but worth investigating separately.

@Touxten
Copy link
Copy Markdown
Contributor

Touxten commented Apr 2, 2026

One thing noticed: when uninstalling and reinstalling the module, the Forecast widget moves to the bottom of the dashboard instead of staying in its original position. This seems unrelated to this PR but worth investigating separately.

That is what is expected

@SiraDIOP
Copy link
Copy Markdown

SiraDIOP commented Apr 2, 2026

Got it, thanks for the clarification!

@SiraDIOP SiraDIOP removed their assignment Apr 2, 2026
@SiraDIOP SiraDIOP added QA ✔️ Status: Check done, Code approved and removed Waiting for QA Status: Action required, Waiting for test feedback labels Apr 2, 2026
@SiraDIOP SiraDIOP self-assigned this Apr 2, 2026
@Touxten
Copy link
Copy Markdown
Contributor

Touxten commented Apr 2, 2026

The CI is red; we need to update it.
The PR is okay; I'll merge it.

Thank you @SiraDIOP && @cnavarro-prestashop

@Touxten Touxten merged commit b0376d4 into PrestaShop:dev Apr 2, 2026
30 of 34 checks passed
@github-project-automation github-project-automation Bot moved this from To be tested to Merged in PR Dashboard Apr 2, 2026
@ps-jarvis ps-jarvis moved this from Merged to To be tested in PR Dashboard Apr 2, 2026
@cnavarro-prestashop cnavarro-prestashop deleted the fix/graph-undefined branch April 2, 2026 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

QA ✔️ Status: Check done, Code approved

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

10 participants