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

fix a bug where false would not be detected as parameter false #278

Merged
merged 8 commits into from
Mar 26, 2024

Conversation

Freundschaft
Copy link
Contributor

@Freundschaft Freundschaft commented Mar 25, 2024

realized that if shouldFocus is set to false, then it will not be set at all in the open call, so added distinguisher from null, sorry for the oversight!

@@ -98,7 +98,7 @@ export const InfoWindow = (props: PropsWithChildren<InfoWindowProps>) => {
openOptions.anchor = anchor;
}

if (shouldFocus) {
if (shouldFocus === true || shouldFocus === false) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the most accurate test here would be "has this been set in the props", so I think it should be if (shouldFocus !== undefined) { ... }. Am I missing something?

Copy link
Contributor Author

@Freundschaft Freundschaft Mar 25, 2024

Choose a reason for hiding this comment

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

in case its used in typescript that probably holds true,
what I committed is just in case you use it with vanilla javascript, then it makes sure its a boolean, aka is either true or false
I dont think it makes a difference, but if you object, we can change it to your suggestion

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just think we shouldn't invent our own rules here, if the google maps API will accept 0 and 1 (it probably does), so should we.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fair enough ill make the change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed now :)

@usefulthink usefulthink merged commit 2f4b508 into visgl:main Mar 26, 2024
2 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.

None yet

2 participants