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 Heatmap Example #19

Merged
merged 9 commits into from
Mar 5, 2019
Merged

Conversation

barakplasma
Copy link
Contributor

@barakplasma barakplasma commented Feb 11, 2019

Screenshots:
screen shot 2019-02-11 at 11 27 41
screen shot 2019-02-11 at 11 28 25

This PR adds a heatmap example to the project.
Complements my PR to improve documentation for the feature: google-map-react/google-map-react#716

@itsmichaeldiego
Copy link
Member

@barakplasma Hey! Thank you so much, It is looking good! Just be careful you added package-lock.json and here we're using yarn instead., if you could fix that and tell me I will merge this asap.

Project uses yarn
@barakplasma
Copy link
Contributor Author

@itsmichaeldiego I deleted the package-lock.json . Do you want me to update the yarn.lock?

src/index.js Outdated
@@ -28,6 +29,7 @@ ReactDOM.render(
<Route path={`${defaultPath}default`} component={Main} />
<Route path={`${defaultPath}searchbox`} component={SearchBox} />
<Route path={`${defaultPath}autocomplete`} component={Autocomplete} />
<Route path={`${defaultPath}heatmap`} component={Heatmap} />
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure if I understood what you meant by align, so I sorted the import lines by length in d9ff68f

src/index.js Outdated
@@ -7,6 +7,7 @@ import Home from './Home';
import Main from './examples/Main';
import SearchBox from './examples/Searchbox';
import Autocomplete from './examples/Autocomplete';
import Heatmap from './examples/Heatmap';
Copy link
Member

Choose a reason for hiding this comment

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

Can we align it like the rest? Sorry, OCD 😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure if I understood what you meant by align, so I sorted the import lines by length in d9ff68f

<GoogleMap
defaultZoom={10}
defaultCenter={LOS_ANGELES_CENTER}
heatmapLibrary
Copy link
Member

Choose a reason for hiding this comment

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

Can we use instead:

    bootstrapURLKeys={{
    key: 'xxxxxxxxxxxxxx',
    libraries: ['visualization'],
  }}

Due to google-map-react/google-map-react#673 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in 8eab280

@itsmichaeldiego
Copy link
Member

@barakplasma Hey man! First let me say this is looking great now, I just left two really minor comments and one where I want to hear what you think: #19 (comment)

@barakplasma
Copy link
Contributor Author

I'll correct the line breaks soon in order to pass CI

@barakplasma
Copy link
Contributor Author

@itsmichaeldiego sorry for the delay. I changed the line endings in order to pass eslint. This PR and google-map-react/google-map-react#716 (comment) seem ready to me for merge.

@itsmichaeldiego itsmichaeldiego merged commit 6bd00be into google-map-react:master Mar 5, 2019
@barakplasma
Copy link
Contributor Author

Thanks @itsmichaeldiego !

I just want to remind you to run

$ npm run deploy

from the master branch in order to deploy the Heatmap Example to the gh-pages site. Currently, the demo doesn't have the heatmap example.

Alternatively, I can make another PR with that change for the gh-pages branch.

@itsmichaeldiego
Copy link
Member

@barakplasma Yeah I did not have time to publish, its published now! Let me know if it works for you

@itsmichaeldiego
Copy link
Member

itsmichaeldiego commented Mar 6, 2019

@barakplasma BTW you might want to create a PR to add the example into the example list in README.md

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