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

Mapbox Map component #896

Merged
merged 13 commits into from
Aug 15, 2018
Merged

Mapbox Map component #896

merged 13 commits into from
Aug 15, 2018

Conversation

kpuputti
Copy link
Contributor

@kpuputti kpuputti commented Aug 13, 2018

This page implements a version of the Map component (used in ListingPage) using Mapbox.

Screenshots

Static map with marker

screen shot 2018-08-14 at 15 22 42

Dynamic map with marker

screen shot 2018-08-14 at 15 23 03

Static map with custom marker

screen shot 2018-08-14 at 15 23 49

Dynamic map with custom marker

screen shot 2018-08-14 at 15 24 01

Static map with fuzzy location circle

screen shot 2018-08-14 at 15 25 04

Dynamic map with fuzzy location circle

screen shot 2018-08-14 at 15 25 14

@kpuputti kpuputti force-pushed the mapbox-map branch 2 times, most recently from 7ffe33d to 63dbca9 Compare August 14, 2018 12:04
Copy link
Contributor

@lyyder lyyder left a comment

Choose a reason for hiding this comment

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

Nice! Check the comments and remember to add a changelog entry.

return markerOverlay(center);
};

class StaticMapboxMap extends Component {
Copy link
Contributor

@lyyder lyyder Aug 14, 2018

Choose a reason for hiding this comment

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

Could this be a functional component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I can change to a function. But always creating a class also has the upside of not having to indent the whole component (and generating lots of diff) when it required state of lifecycle methods.

@@ -3,7 +3,8 @@ import { bool, number, object, string } from 'prop-types';
import classNames from 'classnames';
import { propTypes } from '../../util/types';
import config from '../../config';
import { StaticMap, DynamicMap, isMapsLibLoaded } from './GoogleMap';
// import { StaticMap, DynamicMap, isMapsLibLoaded } from './GoogleMap';
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this comment something we'd leave to the code as an example on how to change the map implementation. Or remove when the map migration is complete?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, the Mapbox one should be commented out and the Google one in use. I'll fix that.

I'd leave the Mapbox imports as comments so we can easily test those while doing the whole map provider migration. We can decide to drop them when putting it all together in the end.

@@ -45,8 +45,10 @@ export class Map extends Component {
/>
) : (
<DynamicMap
containerElement={<div className={classes} onClick={this.onMapClicked} />}
Copy link
Contributor

Choose a reason for hiding this comment

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

Where has the onClick gone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wasn't used at all, there wasn't even a onMapClicked method that it was calling. It was probably copy pasted from SearchMap that had such method.

@kpuputti kpuputti merged commit 5e9fb02 into master Aug 15, 2018
@kpuputti kpuputti deleted the mapbox-map branch August 15, 2018 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants