-
Notifications
You must be signed in to change notification settings - Fork 34
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 Map Popups (and focus traps) #777
Fix Map Popups (and focus traps) #777
Conversation
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.
Small but crucial nitpick
packages/map-popup/src/index.tsx
Outdated
@@ -101,7 +101,7 @@ function entityIsStation(entity: Entity): entity is Station { | |||
/** | |||
* Renders a map popup for a stop, scooter, or shared bike | |||
*/ | |||
export function MapPopup({ closePopup = null, configCompanies, entity, getEntityName, setLocation, setViewedStop }: Props): JSX.Element { | |||
export function MapPopup({ closePopup = () => null, configCompanies, entity, getEntityName, setLocation, setViewedStop }: Props): JSX.Element { |
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.
export function MapPopup({ closePopup = () => null, configCompanies, entity, getEntityName, setLocation, setViewedStop }: Props): JSX.Element { | |
export function MapPopup({ closePopup = () => void, configCompanies, entity, getEntityName, setLocation, setViewedStop }: Props): JSX.Element { |
Should this be void instead?
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.
This isn't a type it's a default!
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.
Yes but look at the type! It returns void
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.
We have a noOp function you can import
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.
This looks great! But CI seems insanely broken? What's going on
Fixes IDs that are breaking due to stop codes, passes
closePopup
toMapPopup
throughotp2-tile-layer
, prevent crash on nullclosePopup