Skip to content

Fix the link on the upstream chart legend #3606

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Sep 22, 2025
Merged

Fix the link on the upstream chart legend #3606

merged 6 commits into from
Sep 22, 2025

Conversation

rdwebdesign
Copy link
Member

What does this PR aim to accomplish?

When 2 or more upstream servers have the same name, both links on the chart legend point to the same IP:

Upstreams

Each link should point to the correct IP and show different queries, but the issue happens because the current code uses an object containing the server name (label) as key and the value is the IP, but when there are 2 items with the same key, the first one is overwritten and its IP is lost.

How does this PR accomplish the above?

Using an array of IPs with indexes will avoid the issue.

This PR also adds the IP to the legend tooltip, to make easier for users to identify different DNS servers, even when they have the same name:

image

Link documentation PRs if any are needed to support this PR:

none

By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code and I have tested my changes.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)
  6. I have checked that another pull request for this purpose does not exist.
  7. I have considered, and confirmed that this submission will be valuable to others.
  8. I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  9. I give this submission freely, and claim no ownership to its content.
  • I have read the above and my PR is ready for review. Check this box to confirm

@rdwebdesign
Remove unnecessary code
52441b7 
The outer `if` (line 92) already guarantees only 2 possible values.
If `isQueryTypeChart` is false, `isForwardDestinationChart` must be true.

Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
@rdwebdesign
Use an array to store upstream server IPs
48666e1 
This will avoid overwritting the IP when more than one upstream DNS server uses the same name

Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
@rdwebdesign rdwebdesign requested a review from a team as a code owner September 16, 2025 02:00
@rdwebdesign
Show the upstream server IP on the title tooltip
af471be 
Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
@rdwebdesign rdwebdesign force-pushed the fix/upstream_chart_legend_link branch from 1478dbc to af471be Compare September 16, 2025 02:03
@rdwebdesign rdwebdesign mentioned this pull request Sep 16, 2025
Open
@yubiuser
Copy link
Member

This is really good.

Could you also try to fix #3 I've found here

If one comes from the dashboard, the the filter does not pre-select the existing entry in the drop-down but creates a new one.

Image

@rdwebdesign
Copy link
Member Author

Hmmm, I hadn't noticed that item 3 in your previous comment...

I will try to fix this tonight.

@rdwebdesign
Include the upstream DNS server name to the link, when needed
ca57bcf 
The link must send the "upstream" parameter in exactly the same format used
by the "suggestions" API.
The format is: "upstream=<IP>#<port> (<servername>)".
This will ensure that when a link is clicked, the correct server name will
be highlighted in the SELECT element on the queries.lp page and no other
OPTION element will be created.

Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
@rdwebdesign
Copy link
Member Author

I already changed the code.

yubiuser
yubiuser reviewed Sep 18, 2025
View reviewed changes
Copy link
Member

@yubiuser yubiuser left a comment

Choose a reason for hiding this comment

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

That's working really well now.
Last nitpick: The tooltip of the doughnut chart contains name#port: percentage but does not contain IP. So for the same upstream name one can only distinguish by color. Could you include the IP here as well?

@rdwebdesign
Copy link
Member Author

I'm not sure if we need to include the IP to the graphic tooltip.

image image

I think the tooltip will be too wide if the server name and IP are both long.

And adding a line break doesn't look good:
image

@yubiuser
Copy link
Member

Yeah, I feared this. But without including the IP the only way to distinguish the same names is by color - which affects accessibility :-/

@rdwebdesign
Copy link
Member Author

rdwebdesign commented Sep 19, 2025 •
edited
Loading

Maybe we should add a more distinguishable effect, like this:

hover_effect

@yubiuser
Copy link
Member

Nice!

@rdwebdesign
Add offset effect on hover to the doughnut charts
ec5f8b7 
We need to add a small padding to avoid "clipping" the arc/slice.
This happens because when an arc/slice expands, it grows beyond the canvas
limits and get clipped.

Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
@rdwebdesign rdwebdesign force-pushed the fix/upstream_chart_legend_link branch 2 times, most recently from e754d4f to 3959ded Compare September 19, 2025 20:09
@rdwebdesign
Trigger the offset effect when the mouse is over a legend item
c6a2e85 
Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
@rdwebdesign rdwebdesign force-pushed the fix/upstream_chart_legend_link branch from 3959ded to c6a2e85 Compare September 19, 2025 20:11
yubiuser
yubiuser reviewed Sep 21, 2025
View reviewed changes
Copy link
Member

@yubiuser yubiuser left a comment

Choose a reason for hiding this comment

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

I observed an interesting effect: the sensitive 'mouseover' area is as big as the longest element in the the whole list. For shorter elements, this means the effect will also apply even when the mouse is not above the text.

Image

@rdwebdesign
Copy link
Member Author

rdwebdesign commented Sep 21, 2025 •
edited
Loading

All elements of each legend have the same width:
image

This is exactly what we have in master and development branches (you can try to click on the right on shorter item to test)

I really don't think this is an issue and this is just cosmetics.

If we decide to change it, we can open a new PR to change the CSS style, to show a background color, border, whatever...

Maybe something like this:
image

yubiuser
yubiuser approved these changes Sep 22, 2025
View reviewed changes
Copy link
Member

@yubiuser yubiuser left a comment

Choose a reason for hiding this comment

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

I think it's only a small detail, no need to fix it now.

@rdwebdesign rdwebdesign merged commit 91346f4 into development Sep 22, 2025
11 checks passed
@rdwebdesign rdwebdesign deleted the fix/upstream_chart_legend_link branch September 22, 2025 07:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yubiuser yubiuser yubiuser approved these changes

@DL6ER DL6ER Awaiting requested review from DL6ER

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@rdwebdesign @yubiuser