refactor docs - navigable and easy-to-add examples by wwwtyro · Pull Request #41 · visgl/react-map-gl

wwwtyro · 2015-12-18T01:27:15Z

No description provided.

wwwtyro · 2015-12-18T01:28:57Z

@vicapow Hope you like it! 🎉

vicapow · 2015-12-18T01:45:41Z

index.js

I think we should use React to create the sidebar if only to be consistent with the the rest of the project.

wwwtyro · 2015-12-18T02:03:45Z

Thanks for the review! I'll try to fix it up tomorrow evening.

If you're at all dissatisfied with the direction I've taken here, I'm happy to refactor it so that it's more in line with what you envisioned - I realize it's a significant departure from the original.

vicapow · 2015-12-18T02:21:01Z

index.css

what do you think about Helvetica Neue, instead?

Know where I can source a Helvetica Neue web font from? I've not found a free one.

vicapow · 2015-12-18T02:22:02Z

It's looking good so far! I do have a some feedback. still going through.

vicapow · 2015-12-18T02:27:34Z

I've been trying to keep build commits of bundle.js separate from regular changes.

vicapow · 2015-12-18T02:59:48Z

index.css

can you try #1FBAD6?

wwwtyro · 2015-12-18T04:35:55Z

Excellent, thanks for all the feedback. 👍 I'll try to get through these tomorrow.

wwwtyro · 2015-12-19T05:23:35Z

I addressed some issues and rebased to separate out the bundle rebuild into a new commit. I'm still working on upgrading react-map-gl to 0.6.1, but I've hit a snag rendering the overlays (they don't appear, no errors - see https://github.com/uber/react-map-gl/pull/41/files#diff-386b72445554b0b2557c2637ec3aea34R95 for my attempt). Unfortunately I've run out of time this evening, and am travelling this weekend, so won't be able to start again until Monday.

Have a great weekend!

vicapow · 2015-12-22T04:28:47Z

@wwwtyro awesome progress.

you just need to swap latitude and longitude. Sorry about that. We changed the API when Mapbox decided to swap them too so you can blame them, lol.

diff --git a/content/scatterplot/scatterplot.js b/content/scatterplot/scatterplot.js
index 839fed4..5efdecb 100644
--- a/content/scatterplot/scatterplot.js
+++ b/content/scatterplot/scatterplot.js
@@ -33,7 +33,6 @@ var stamenMapStyle = require('../../common/stamen-map-style');
 var CodeSnippet = require('../../common/code-snippet.react');
 var Markdown = require('../../common/markdown');

-
 module.exports = React.createClass({

   getInitialState: function getInitialState() {
@@ -53,7 +52,7 @@ module.exports = React.createClass({
         startDragLngLat: null
       },
       locations: Immutable.fromJS(d3.range(2000).map(function _map() {
-        return [37.788 + wiggle(0.01), -122.408 + wiggle(0.01)];
+        return [-122.408 + wiggle(0.01), 37.788 + wiggle(0.01), ];
       }))
     };
   },
diff --git a/package.json b/package.json
index beb745a..ab506a8 100644
--- a/package.json
+++ b/package.json
@@ -9,6 +9,7 @@
     "browserify": "^12.0.1",
     "budo": "^6.1.0",
     "d3": "^3.5.8",
+    "global": "^4.3.0",
     "highlight.js": "^8.8.0",
     "immutable": "^3.7.6",
     "js-stylesheet": "0.0.1",

wwwtyro · 2015-12-22T04:42:57Z

Thanks! Yes, figured that out in https://github.com/wwwtyro/react-map-gl/commit/bd1e3a1465dc62f64d1968341c95a2ff9fbbdf47 -- should have left a comment saying so, my bad. Currently working on getting react-router working. :)

vicapow · 2015-12-22T19:49:38Z

oh, nice! both our bad's 😜

wwwtyro · 2015-12-22T21:06:22Z

hahaha

wwwtyro · 2015-12-22T21:43:48Z

Alright, this one is ready for another review (no hurry though, enjoy your break!). The actual wording of the docs probably needs an expert eye to make sure I'm getting things right for the 0.6.x API.

vicapow · 2015-12-28T22:10:08Z

Thanks, @wwwtyro! This is all looking good so far. I forgot to mention earlier but can you make sure npm run lint passes before I accept? checkout: uber-standard for more context.

wwwtyro · 2015-12-29T19:31:00Z

No problem - done!

vicapow · 2015-12-29T20:10:03Z

@wwwtyro still seeing a lot of output from npm run lint but it's all only indentation related. Maybe it's something about your editor?

react-map-gl ((c610a79...)) -> rm -rf node_modules && npm install && npm run lint
Error: Use Uber JavaScript Standard Style (https://github.com/uber/standard)
  /Users/victorpowell/Documents/projects/react-map-gl/common/code-snippet.react.js:39:6: Expected indentation of 6 characters. (indent)
  /Users/victorpowell/Documents/projects/react-map-gl/common/code-snippet.react.js:40:6: Expected indentation of 6 characters. (indent)
...

wwwtyro · 2015-12-29T20:16:48Z

Weird, maybe I'm running the linter wrong or something, I'm error-free right now. The file you're running against giving that error looks like this?

vicapow · 2015-12-29T20:32:38Z

@wwwtyro Yep. The file looks the same. uber-standard (and standard) both enforce 2 space indentation. Maybe you have a miscellaneous .eslintrc mucking things up? uber-standard --format can also fix the indentation issues automatically:

react-map-gl ((c610a79...)) -> uber-standard common/markdown.js
Error: Use Uber JavaScript Standard Style (https://github.com/uber/standard)
  /Users/victorpowell/Documents/projects/react-map-gl/common/markdown.js:34:6: Expected indentation of 6 characters. (indent)
react-map-gl ((c610a79...)) -> uber-standard --format common/markdown.js
react-map-gl ((c610a79...)) -> uber-standard common/markdown.js

wwwtyro · 2015-12-29T21:17:35Z

I've tried a number of things, but it keeps insisting on 4-space indentation. Looking at this in uber-standard and this eslint doc, it seems like it really does want 4 spaces. I'm kicking off a VM right now to redo all the things and make sure I'm not mucking something up.

vicapow · 2015-12-29T21:51:30Z

@wwwtyro I don't think indent: [2, 4] is a range. Looking at the source code, https://github.com/eslint/eslint/blob/master/lib/rules/indent.js#L57 it seems the first argument is exactly the number of spaces it wants. Maybe try updating uber-standard for the project to the latest version and try again? Should be version 5.1.0.

npm install uber-standard@latest --save

I get slightly different output when I do that:

react-map-gl ((c610a79...)) -> npm run lint

> react-map-gl-docs@0.0.0 lint /Users/victorpowell/Documents/projects/react-map-gl
> uber-standard

Error: Use Uber JavaScript Standard Style (https://github.com/uber/standard)
  /Users/victorpowell/Documents/projects/react-map-gl/common/code-snippet.react.js:30:5: Expected indentation of 2 space characters but found 4. (indent)
  /Users/victorpowell/Documents/projects/react-map-gl/common/code-snippet.react.js:32:5: Expected indentation of 2 space characters but found 4. (indent)

I also think .eslintrc files are resolved recursively through every parent directory so maybe that's what's happening here.

wwwtyro · 2015-12-29T21:57:48Z

The eslint docs say:

It takes an option as the second parameter which can be "tab" for tab-based indentation or a positive number for space indentations.

err... are those the right docs?

I looked for an errant .eslintrc file but didn't find one. My VM should be ready to test this soon.

wwwtyro · 2015-12-29T22:25:05Z

I also get different errors with 5.1.0, but it's still looking for 4 spaces (it looks like it's just more particular about what lines it wants indented properly).

I tried it on a VM, and it still wants 4 spaces. Not sure what else to check. :\ Is it possible you have an errant .eslintrc file somewhere? Like, if you copy react-map-gl into /tmp and lint it from there, uber-standard still gives errors indicating it wants 2-space indentation?

Sorry, I'm sure this is the last thing you wanted to spend time on today! :O

wwwtyro · 2015-12-29T23:06:14Z

It looks like it's looking for an .editorconfig file to define the indentation:

https://github.com/uber/standard/blob/master/index.js#L72

which is 2 in the .editorconfig file included in uber-standard.

editorconfig-get-indent isn't hosted on github, but I printed out what it found when looking for .editorconfig, and it didn't find anything, in which case it defaults to 4. Is it possible you have an .editorconfig file lurking somewhere in your tree above react-map-gl? As the noob here, I'm loath to point the finger at your env, but I'm not sure I can otherwise interpret the fresh VM experiment I ran.

Either way, it seems that the intent is probably to use 2 spaces for indentation, so I'll do that for now.

vicapow · 2015-12-30T03:41:28Z

@wwwtyro That's it! Nice thinking firing up a VM. Here's what my .editorconfig file looks like for reference. Could you use this for now until we fix this issue with uber-standard? I filed a ticket with uber-standard at: uber/standard#49 and will try to dig into this when I get some time.

# EditorConfig: http://editorconfig.org

root = true

[*]
indent_style = space
indent_size = 2
end_of_line = lf
charset = utf-8
trim_trailing_whitespace = true
insert_final_newline = true

refactor docs - navigable and easy-to-add examples

wwwtyro · 2015-12-30T03:49:34Z

Whew, I was worried I was starting to be more annoying than useful! :D

Roger wilco re: .editorconfig. 👍

Thanks for your patience & the merge!

vicapow · 2016-01-05T00:26:37Z

I went and added a typical .editorconfig to the project, both the master and gh-pages branch. That should at least prevent this from popping up for this project in the future.

9fd6416
3996245

vicapow reviewed Dec 18, 2015
View reviewed changes

index.js Outdated

Copy link

Contributor

vicapow Dec 18, 2015

Choose a reason for hiding this comment

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

I think we should use React to create the sidebar if only to be consistent with the the rest of the project.

vicapow reviewed Dec 18, 2015
View reviewed changes

index.css Outdated

Copy link

Contributor

vicapow Dec 18, 2015

Choose a reason for hiding this comment

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

can you try #1FBAD6?

wwwtyro added 2 commits December 18, 2015 23:14

refactor docs - navigable and easy-to-add examples

7678f99

rebuild bundle

8c8b3b9

wwwtyro added 5 commits December 21, 2015 17:50

scatterplot example working under 0.6.1

bd1e3a1

draggable points example working under 0.6.1

a6524f5

heatmap overlay working under 0.6.1

2e230de

restore hashlink behavior

e69b38e

default to Helvetica Neue

d5cab05

wwwtyro added 4 commits December 22, 2015 12:58

things working with react-router

9149c61

minor cleanup of examples

e1cc7c4

clean up third-party overlay docs

72af4b4

minor doc cleanup

abd1065

wwwtyro added 2 commits December 22, 2015 15:37

add draggable points documentation

2bc3681

rebuild bundle.js

160f3c1

wwwtyro added 2 commits December 29, 2015 13:29

conform to uber-standard

d86bc36

update bundle.js

c610a79

update uber-standard to 5.1.0 and use 2-space indentation

945f589

vicapow mentioned this pull request Dec 30, 2015

indentation is not actually 2 spaces uber/standard#49

Open

vicapow added a commit that referenced this pull request Dec 30, 2015

Merge pull request #41 from wwwtyro/refactor-docs

5f84368

refactor docs - navigable and easy-to-add examples

vicapow merged commit 5f84368 into visgl:gh-pages Dec 30, 2015

Conversation

wwwtyro commented Dec 18, 2015

Uh oh!

wwwtyro commented Dec 18, 2015

Uh oh!

vicapow Dec 18, 2015

Choose a reason for hiding this comment

Uh oh!

wwwtyro commented Dec 18, 2015

Uh oh!

vicapow Dec 18, 2015

Choose a reason for hiding this comment

Uh oh!

wwwtyro Dec 19, 2015

Choose a reason for hiding this comment

Uh oh!

vicapow commented Dec 18, 2015

Uh oh!

vicapow commented Dec 18, 2015

Uh oh!

vicapow Dec 18, 2015

Choose a reason for hiding this comment

Uh oh!

wwwtyro commented Dec 18, 2015

Uh oh!

wwwtyro commented Dec 19, 2015

Uh oh!

vicapow commented Dec 22, 2015

Uh oh!

wwwtyro commented Dec 22, 2015

Uh oh!

vicapow commented Dec 22, 2015

Uh oh!

wwwtyro commented Dec 22, 2015

Uh oh!

wwwtyro commented Dec 22, 2015

Uh oh!

vicapow commented Dec 28, 2015

Uh oh!

wwwtyro commented Dec 29, 2015

Uh oh!

vicapow commented Dec 29, 2015

Uh oh!

wwwtyro commented Dec 29, 2015

Uh oh!

vicapow commented Dec 29, 2015

Uh oh!

wwwtyro commented Dec 29, 2015

Uh oh!

vicapow commented Dec 29, 2015

Uh oh!

wwwtyro commented Dec 29, 2015

Uh oh!

wwwtyro commented Dec 29, 2015

Uh oh!

wwwtyro commented Dec 29, 2015

Uh oh!

vicapow commented Dec 30, 2015

Uh oh!

wwwtyro commented Dec 30, 2015

Uh oh!

vicapow commented Jan 5, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants