Conversation
Made a pannable/zoomable grid on D3
chynu
left a comment
chynu
left a comment
There was a problem hiding this comment.
Haven't actually tested it on my computer though lol. I'm trusting you've tested it on yours .. ! Probably a good idea to try it out on ubuntu or another person's computer before you merge though.
| <!-- basic imports (NEED TO BE LOCALIZED) --> | ||
| <script src="https://ajax.googleapis.com/ajax/libs/jquery/1.12.4/jquery.min.js"></script> | ||
| <script src="http://pixijs.download/release/pixi.min.js"></script> | ||
| <script src="https://d3js.org/d3.v5.min.js"></script> |
There was a problem hiding this comment.
Please add a TODO to change this to be a local import.
There was a problem hiding this comment.
In fact it may be a good idea to make an issue alongside this. We should have everything installed via npm.
| <!-- basic imports --> | ||
| <!-- basic imports (NEED TO BE LOCALIZED) --> | ||
| <script src="https://ajax.googleapis.com/ajax/libs/jquery/1.12.4/jquery.min.js"></script> | ||
| <script src="http://pixijs.download/release/pixi.min.js"></script> |
There was a problem hiding this comment.
If I'm not mistaken, we won't need Pixi.js if we've completely ported to the new gridview in d3.
| // this.setup(background); | ||
| // } | ||
|
|
||
| console.log("using white background"); |
There was a problem hiding this comment.
Please remove all console outputs, here and below.
Remember, whenever you make a PR you're planning on merging to develop, which is our "latest final working version." Which means we shouldn't have debugging output. Keep that for just testing.
| import Scenarios from './components/scenarios.js'; | ||
| import AddBot from './components/addbot.js'; | ||
| import GridView from './components/gridview.js'; | ||
| import GridView from './components/gridview2.js'; |
There was a problem hiding this comment.
If you're completely done with the old version of gridview, you can delete it. There's no need to have vestigial code in develop. (Once you've deleted it you can rename gridview2 to gridview, etc.)
chynu
left a comment
chynu
left a comment
There was a problem hiding this comment.
I tested it on my computer and it works! Good job. :)
Two things:
- If there are no bots on the field, there shouldn't be a "find bots" button. Perhaps you should make another button called "return to origin" or "center" or something like that.
- Before you merge, please check this with the vision system and see if the units are reasonably matched to IRL because it'd be weird if one inch corresponded to 500 of whatever units they are. (Also yeah, what units are those?)
Also -- It's PR etiquette(?) to comment "Done" or something on each of the PR comments which requests for a change, alongside any updates if there need be any. FYI, from now on :>
Make sure to comment about calibrating with vision on the PR here, as well !
| @@ -1,5 +1,7 @@ | |||
| var React = require('react'); | |||
| var ReactDOM = require('react-dom'); | |||
| //var d3 = require('d3'); | |||
There was a problem hiding this comment.
Is this no longer necessary? In which case you should just remove them instead of keeping the comments. If you're going to comment out something and it's not meant to be permanent, then you should delete it or at least add a TODO to take care of it in the line above the comment.
2 participants
Made a pannable and zoomable grid using d3!