Skip to content

Issue 1989: Convert null values to empty strings in popup and tooltip #2134

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

Merged
merged 1 commit into from
Apr 22, 2025

Conversation

hansthen
Copy link
Collaborator

@hansthen hansthen commented Apr 18, 2025

Close #1989

@hansthen hansthen requested a review from Conengmo April 18, 2025 07:47
@hansthen hansthen changed the title Special case null values in popup and tooltip Issue 1989: Convert null values to empty strings in popup and tooltip Apr 19, 2025
@hansthen hansthen requested a review from ocefpaf April 19, 2025 05:32
@hansthen
Copy link
Collaborator Author

@ocefpaf can you have a look?

@ocefpaf
Copy link
Member

ocefpaf commented Apr 22, 2025

Can we add a test to avoid regression? Maybe ask in #1989 for some code/file we can use?

@hansthen
Copy link
Collaborator Author

Can we add a test to avoid regression? Maybe ask in #1989 for some code/file we can use?

I wrote a small program to visually test this. But since this is behavior in the generated code, it is difficult to test with our current tests frameworks.

I have a few ideas on how to make visual snapshots and compare against those during tests. Is it okay if I work on snapshot / regression testing in a new branch?

@ocefpaf
Copy link
Member

ocefpaf commented Apr 22, 2025

I have a few ideas on how to make visual snapshots and compare against those during tests.

We can use https://github.com/matplotlib/pytest-mpl. I had some success with it in the past. However, we can try a text based text that checks if the HTML has null in those fields.

Is it okay if I work on snapshot / regression testing in a new branch?

Sure. IMO it is OK to merge as-is and we can open an issue for further testing here.

@hansthen
Copy link
Collaborator Author

However, we can try a text based text that checks if the HTML has null in those fields.

It's not so easy. The null values are still there. We just filter them in javascript when we generate the popup.

    let handleObject = feature => {
        if (feature === null) {
            return '';
        } else if (typeof(feature)=='object') {
            return JSON.stringify(feature);
        } else {
            return feature;
        }
    }

I can of course create to test that specific string, but that would feel a bit pointless.

@hansthen
Copy link
Collaborator Author

I created an issue for the regression tests. #2140

@ocefpaf
Copy link
Member

ocefpaf commented Apr 22, 2025

I can of course create to test that specific string, but that would feel a bit pointless.

Indeed. Let's merge and focus on the image comparison test in the future.

@hansthen hansthen merged commit 1043da8 into python-visualization:main Apr 22, 2025
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Geojsonpopup change when cell is empty
2 participants