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

add shouldfocus related to #253 #254

Merged
merged 5 commits into from
Mar 12, 2024
Merged

add shouldfocus related to #253 #254

merged 5 commits into from
Mar 12, 2024

Conversation

Freundschaft
Copy link
Contributor

Add option to set shouldFocus

@usefulthink
Copy link
Collaborator

Awesome, thanks! I think the shouldFocus prop needs to be added to the InfoWindowProps type, since that only includes the google.maps.InfoWindowOptions, not the google.maps.InfoWindowOpenOptions.

Also please check the formatting with prettier (npm run test:prettier to check and npx prettier --write ./src/components/info-window.tsx to fix).

@Freundschaft
Copy link
Contributor Author

whoops youre right ill fix and resubmit

@Freundschaft
Copy link
Contributor Author

just made the changes, should be fine now I think

@Freundschaft
Copy link
Contributor Author

Could it also be considered to add InfoWindowOpenOptions as a prop entirely in case the interface changes in the future?

@usefulthink
Copy link
Collaborator

Could it also be considered to add InfoWindowOpenOptions as a prop entirely in case the interface changes in the future?

I don't think that makes a lot of sense, since the open-options have to always be considered separately as you did here. So new options would just cause unexpected behaviour until they are implemented.

Copy link
Collaborator

@usefulthink usefulthink left a comment

Choose a reason for hiding this comment

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

except for the type it's perfect.

src/components/info-window.tsx Outdated Show resolved Hide resolved
@usefulthink usefulthink merged commit c83ea37 into visgl:main Mar 12, 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