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

A solution to issue #647 #651

Merged
merged 3 commits into from
Oct 14, 2015
Merged

A solution to issue #647 #651

merged 3 commits into from
Oct 14, 2015

Conversation

brianbancroft
Copy link
Contributor

Here are a few lines that will solve this problem completely.

@jgravois
Copy link
Contributor

I have to catch a 🚤 home from morocco, but I'll be able to sit down at a computer and take a look at this in a couple days.

thx @brianbancroft!

@patrickarlt
Copy link
Contributor

@brianbancroft this solution looks good. it looks like the travis CI tests are failing when trying to lint this.

To keep the code style consistent could you resolve these? https://travis-ci.org/Esri/esri-leaflet/builds/84613490#L159-L161

@jgravois
Copy link
Contributor

you can lint the code locally with npm run lint and run the tests locally with npm test after you make your own changes to make sure the continuous integration tests that run automatically when a pull request is submitted pass.

@brianbancroft
Copy link
Contributor Author

Hey gents, thanks for the heads up. I've looked into this and changed the spacing issues on the lines that @patrickarlt kindly pointed me out to, and I think I've given a proper pull request. There are different problems, but I haven't been able to decipher which of this log indicates what has gone wrong.

Cleaned up some spacing issues
@patrickarlt
Copy link
Contributor

It looks like the Travis CI build randomly disconnected, which happens sometimes for reasons I have been hopelessly trying to debug forever. I restarted the build and it looks good now.

jgravois added a commit that referenced this pull request Oct 14, 2015
@jgravois jgravois merged commit 780c960 into Esri:master Oct 14, 2015
@jgravois
Copy link
Contributor

yay! 💯 thanks for sticking with it @brianbancroft!

@brianbancroft
Copy link
Contributor Author

No worries guys 👍

Thanks for helping me get the foot in the door. I have a lot more beginner-level questions to ask on topics ranging from specifics to "what next". Would pushing them on the main issue page be a good forum?

@jgravois
Copy link
Contributor

we're always happy to help if things aren't working as you expect. that being said, you might attract even more eyes in gis.se or the Esri Leaflet Place on Geonet if its more of a 'how to' than a suspected bug.

@patrickarlt
Copy link
Contributor

@brianbancroft @jgravois @kentcdodds I just want to share some of my thoughts on watching this whole process go down.

First thanks to @jgravois for putting in the time to actually pull this together and document the issue at hand. Ultimately I think everyone learned a whole bunch of stuff not just @brianbancroft. This is a great exercise for maintainers to test their installation and contributing instructions. For example I would have never thought i necessary to need to document or test the dev process against multiple versions of Node. Having a real watch + recompile + dev server solution seems like a real need.

Overall this was pretty fantastic to watch and I'm glad that @brianbancroft learned a whole bunch. I did too so hopefully the next time will be easier for someone.

@kentcdodds
Copy link

Yay! This is awesome. Glad it went so well.

jgravois added a commit to jgravois/esri-leaflet that referenced this pull request Apr 23, 2022
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.

4 participants