-
Notifications
You must be signed in to change notification settings - Fork 125
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
Conversation
src/components/info-window.tsx
Outdated
@@ -98,7 +98,7 @@ export const InfoWindow = (props: PropsWithChildren<InfoWindowProps>) => { | |||
openOptions.anchor = anchor; | |||
} | |||
|
|||
if (shouldFocus) { | |||
if (shouldFocus === true || shouldFocus === false) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed now :)
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!