GitHub PullRequestReviewCommentEvent at 2014-09-17 21:50:51
Repository: plottable
Actor: bluong
Payload: {"action":"created","comment":{"url":"https://api.github.com/repos/palantir/plottable/pulls/comments/17696843","id":17696843,"diff_hunk":"@@ -17,10 +17,149 @@ export module Interaction {\n      * Whether or not dragBox has been rendered in a visible area.\n      */\n     public boxIsDrawn = false;\n+    public resizePadding = 10;\n+    /**\n+     * The currently selected area, which can be different from the are the user has dragged.\n+     */\n+    public selection: SelectionArea;\n+    /**\n+     * True if box is resizing on the X dimension.\n+     */\n+    public isResizingX = false;\n+    /**\n+     * True if box is resizing on the Y dimension.\n+     */\n+    public isResizingY = false;\n+    /**\n+     * True if box is resizing.\n+     */\n+    public isResizing = false;","path":"src/interactions/drag/dragBoxInteraction.ts","position":27,"original_position":20,"commit_id":"3dff2db11bb1b81b1d45ba37bf376e9170b21459","original_commit_id":"f108922cff9184acfdbd715e819bf0dfd41a0b35","user":{"login":"bluong","id":1448299,"avatar_url":"https://avatars.githubusercontent.com/u/1448299?v=2","gravatar_id":"7b677576ad7299bf8e23f684c2aa358a","url":"https://api.github.com/users/bluong","html_url":"https://github.com/bluong","followers_url":"https://api.github.com/users/bluong/followers","following_url":"https://api.github.com/users/bluong/following{/other_user}","gists_url":"https://api.github.com/users/bluong/gists{/gist_id}","starred_url":"https://api.github.com/users/bluong/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bluong/subscriptions","organizations_url":"https://api.github.com/users/bluong/orgs","repos_url":"https://api.github.com/users/bluong/repos","events_url":"https://api.github.com/users/bluong/events{/privacy}","received_events_url":"https://api.github.com/users/bluong/received_events","type":"User","site_admin":false},"body":"So just wondering, can you identify a usecase for knowing such information?  If there is such a usecase, then sure I can imagine a getter to satisfy this behavior.  However, in the case of the setter, why should this variable be allowed to change without you knowing?  From what I gather you are essentially following a determined code flow with these variables, but what happens if this flow is nondeterministic since the user can modify these variables?","created_at":"2014-09-17T21:50:51Z","updated_at":"2014-09-17T21:50:51Z","html_url":"https://github.com/palantir/plottable/pull/863#discussion_r17696843","pull_request_url":"https://api.github.com/repos/palantir/plottable/pulls/863","_links":{"self":{"href":"https://api.github.com/repos/palantir/plottable/pulls/comments/17696843"},"html":{"href":"https://github.com/palantir/plottable/pull/863#discussion_r17696843"},"pull_request":{"href":"https://api.github.com/repos/palantir/plottable/pulls/863"}}},"pull_request":{"url":"https://api.github.com/repos/palantir/plottable/pulls/863","id":20102826,"html_url":"https://github.com/palantir/plottable/pull/863","diff_url":"https://github.com/palantir/plottable/pull/863.diff","patch_url":"https://github.com/palantir/plottable/pull/863.patch","issue_url":"https://api.github.com/repos/palantir/plottable/issues/863","number":863,"state":"open","locked":false,"title":"Dragbox resizing when edges are dragged","user":{"login":"fent","id":933490,"avatar_url":"https://avatars.githubusercontent.com/u/933490?v=2","gravatar_id":"3471337d7de26218e5fced27d186f020","url":"https://api.github.com/users/fent","html_url":"https://github.com/fent","followers_url":"https://api.github.com/users/fent/followers","following_url":"https://api.github.com/users/fent/following{/other_user}","gists_url":"https://api.github.com/users/fent/gists{/gist_id}","starred_url":"https://api.github.com/users/fent/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/fent/subscriptions","organizations_url":"https://api.github.com/users/fent/orgs","repos_url":"https://api.github.com/users/fent/repos","events_url":"https://api.github.com/users/fent/events{/privacy}","received_events_url":"https://api.github.com/users/fent/received_events","type":"User","site_admin":false},"body":"(opening this on `develop` branch)\r\n\r\nThis PR implements resizing a dragbox by dragging its edges when it's drawn on screen. The cursor changes appropriately even on first draw.\r\n\r\nhttp://jsfiddle.net/552z0h27/\r\n\r\nThere are some questionable areas I'd like feedback on, specifically on how organized code looks and its simplicity. The code added to `XYDragBoxInteraction.ts` seems like a lot for example.\r\n\r\nThe API for dragging events still works like before, the callbacks the start and end points, even on resizing even if the dragbox is not bounded by those points. For that, there is `DragBoxInteraction.selection` which is a `SelectionArea`.\r\n\r\n`DragBoxInteraction.isResizing` is public, I thought it'd be useful for the user rather than them keeping track themselves. There are some use cases where a user might want to load data for example only on `dragend` and on `drag` only when `isResizing` is `true`, but not on first draw.\r\n\r\nThere is still an error when dragging from the very edge, it sometimes draws a new box instead of resizing. I suspect this happens because the d3 `drag` events only provide `d3.event.x` and `d3.event.y` on the first `drag` event, instead of on `dragstart`. On `drag` the user will have already moved at least a pixel, making the mouse position not close enough for a resize.\r\n\r\nWe can \"fix\" this by binding for the click event, and setting the origin early, inside of `DragInteraction._dragstart()`. Or by lowering the padding at which the cursor is changed, but keeping it slightly bigger from actually checking if it's close enough for resize.\r\n\r\nThoughts?","created_at":"2014-08-21T11:07:31Z","updated_at":"2014-09-17T21:50:51Z","closed_at":null,"merged_at":null,"merge_commit_sha":"6058312dcf38a11459d50790e27c2ef9684af856","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/palantir/plottable/pulls/863/commits","review_comments_url":"https://api.github.com/repos/palantir/plottable/pulls/863/comments","review_comment_url":"https://api.github.com/repos/palantir/plottable/pulls/comments/{number}","comments_url":"https://api.github.com/repos/palantir/plottable/issues/863/comments","statuses_url":"https://api.github.com/repos/palantir/plottable/statuses/3dff2db11bb1b81b1d45ba37bf376e9170b21459","head":{"label":"poptip:dragresize","ref":"dragresize","sha":"3dff2db11bb1b81b1d45ba37bf376e9170b21459","user":{"login":"poptip","id":896930,"avatar_url":"https://avatars.githubusercontent.com/u/896930?v=2","gravatar_id":"fd2741961bf7467ccfcb167d6aa61440","url":"https://api.github.com/users/poptip","html_url":"https://github.com/poptip","followers_url":"https://api.github.com/users/poptip/followers","following_url":"https://api.github.com/users/poptip/following{/other_user}","gists_url":"https://api.github.com/users/poptip/gists{/gist_id}","starred_url":"https://api.github.com/users/poptip/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/poptip/subscriptions","organizations_url":"https://api.github.com/users/poptip/orgs","repos_url":"https://api.github.com/users/poptip/repos","events_url":"https://api.github.com/users/poptip/events{/privacy}","received_events_url":"https://api.github.com/users/poptip/received_events","type":"Organization","site_admin":false},"repo":{"id":22314009,"name":"plottable","full_name":"poptip/plottable","owner":{"login":"poptip","id":896930,"avatar_url":"https://avatars.githubusercontent.com/u/896930?v=2","gravatar_id":"fd2741961bf7467ccfcb167d6aa61440","url":"https://api.github.com/users/poptip","html_url":"https://github.com/poptip","followers_url":"https://api.github.com/users/poptip/followers","following_url":"https://api.github.com/users/poptip/following{/other_user}","gists_url":"https://api.github.com/users/poptip/gists{/gist_id}","starred_url":"https://api.github.com/users/poptip/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/poptip/subscriptions","organizations_url":"https://api.github.com/users/poptip/orgs","repos_url":"https://api.github.com/users/poptip/repos","events_url":"https://api.github.com/users/poptip/events{/privacy}","received_events_url":"https://api.github.com/users/poptip/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/poptip/plottable","description":"A library for creating interactive charts within a grid layout.","fork":true,"url":"https://api.github.com/repos/poptip/plottable","forks_url":"https://api.github.com/repos/poptip/plottable/forks","keys_url":"https://api.github.com/repos/poptip/plottable/keys{/key_id}","collaborators_url":"https://api.github.com/repos/poptip/plottable/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/poptip/plottable/teams","hooks_url":"https://api.github.com/repos/poptip/plottable/hooks","issue_events_url":"https://api.github.com/repos/poptip/plottable/issues/events{/number}","events_url":"https://api.github.com/repos/poptip/plottable/events","assignees_url":"https://api.github.com/repos/poptip/plottable/assignees{/user}","branches_url":"https://api.github.com/repos/poptip/plottable/branches{/branch}","tags_url":"https://api.github.com/repos/poptip/plottable/tags","blobs_url":"https://api.github.com/repos/poptip/plottable/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/poptip/plottable/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/poptip/plottable/git/refs{/sha}","trees_url":"https://api.github.com/repos/poptip/plottable/git/trees{/sha}","statuses_url":"https://api.github.com/repos/poptip/plottable/statuses/{sha}","languages_url":"https://api.github.com/repos/poptip/plottable/languages","stargazers_url":"https://api.github.com/repos/poptip/plottable/stargazers","contributors_url":"https://api.github.com/repos/poptip/plottable/contributors","subscribers_url":"https://api.github.com/repos/poptip/plottable/subscribers","subscription_url":"https://api.github.com/repos/poptip/plottable/subscription","commits_url":"https://api.github.com/repos/poptip/plottable/commits{/sha}","git_commits_url":"https://api.github.com/repos/poptip/plottable/git/commits{/sha}","comments_url":"https://api.github.com/repos/poptip/plottable/comments{/number}","issue_comment_url":"https://api.github.com/repos/poptip/plottable/issues/comments/{number}","contents_url":"https://api.github.com/repos/poptip/plottable/contents/{+path}","compare_url":"https://api.github.com/repos/poptip/plottable/compare/{base}...{head}","merges_url":"https://api.github.com/repos/poptip/plottable/merges","archive_url":"https://api.github.com/repos/poptip/plottable/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/poptip/plottable/downloads","issues_url":"https://api.github.com/repos/poptip/plottable/issues{/number}","pulls_url":"https://api.github.com/repos/poptip/plottable/pulls{/number}","milestones_url":"https://api.github.com/repos/poptip/plottable/milestones{/number}","notifications_url":"https://api.github.com/repos/poptip/plottable/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/poptip/plottable/labels{/name}","releases_url":"https://api.github.com/repos/poptip/plottable/releases{/id}","created_at":"2014-07-27T15:23:16Z","updated_at":"2014-07-26T04:52:51Z","pushed_at":"2014-09-17T20:05:23Z","git_url":"git://github.com/poptip/plottable.git","ssh_url":"git@github.com:poptip/plottable.git","clone_url":"https://github.com/poptip/plottable.git","svn_url":"https://github.com/poptip/plottable","homepage":"palantir.github.io/plottable","size":19237,"stargazers_count":0,"watchers_count":0,"language":null,"has_issues":false,"has_downloads":true,"has_wiki":true,"has_pages":true,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":0,"default_branch":"master"}},"base":{"label":"palantir:develop","ref":"develop","sha":"ff572cac2834bfa070946e59aac4964ca743578f","user":{"login":"palantir","id":303157,"avatar_url":"https://avatars.githubusercontent.com/u/303157?v=2","gravatar_id":"c593136bf7c6a139280354bfc1448a14","url":"https://api.github.com/users/palantir","html_url":"https://github.com/palantir","followers_url":"https://api.github.com/users/palantir/followers","following_url":"https://api.github.com/users/palantir/following{/other_user}","gists_url":"https://api.github.com/users/palantir/gists{/gist_id}","starred_url":"https://api.github.com/users/palantir/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/palantir/subscriptions","organizations_url":"https://api.github.com/users/palantir/orgs","repos_url":"https://api.github.com/users/palantir/repos","events_url":"https://api.github.com/users/palantir/events{/privacy}","received_events_url":"https://api.github.com/users/palantir/received_events","type":"Organization","site_admin":false},"repo":{"id":16149854,"name":"plottable","full_name":"palantir/plottable","owner":{"login":"palantir","id":303157,"avatar_url":"https://avatars.githubusercontent.com/u/303157?v=2","gravatar_id":"c593136bf7c6a139280354bfc1448a14","url":"https://api.github.com/users/palantir","html_url":"https://github.com/palantir","followers_url":"https://api.github.com/users/palantir/followers","following_url":"https://api.github.com/users/palantir/following{/other_user}","gists_url":"https://api.github.com/users/palantir/gists{/gist_id}","starred_url":"https://api.github.com/users/palantir/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/palantir/subscriptions","organizations_url":"https://api.github.com/users/palantir/orgs","repos_url":"https://api.github.com/users/palantir/repos","events_url":"https://api.github.com/users/palantir/events{/privacy}","received_events_url":"https://api.github.com/users/palantir/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/palantir/plottable","description":"A library of modular chart components, built on D3","fork":false,"url":"https://api.github.com/repos/palantir/plottable","forks_url":"https://api.github.com/repos/palantir/plottable/forks","keys_url":"https://api.github.com/repos/palantir/plottable/keys{/key_id}","collaborators_url":"https://api.github.com/repos/palantir/plottable/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/palantir/plottable/teams","hooks_url":"https://api.github.com/repos/palantir/plottable/hooks","issue_events_url":"https://api.github.com/repos/palantir/plottable/issues/events{/number}","events_url":"https://api.github.com/repos/palantir/plottable/events","assignees_url":"https://api.github.com/repos/palantir/plottable/assignees{/user}","branches_url":"https://api.github.com/repos/palantir/plottable/branches{/branch}","tags_url":"https://api.github.com/repos/palantir/plottable/tags","blobs_url":"https://api.github.com/repos/palantir/plottable/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/palantir/plottable/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/palantir/plottable/git/refs{/sha}","trees_url":"https://api.github.com/repos/palantir/plottable/git/trees{/sha}","statuses_url":"https://api.github.com/repos/palantir/plottable/statuses/{sha}","languages_url":"https://api.github.com/repos/palantir/plottable/languages","stargazers_url":"https://api.github.com/repos/palantir/plottable/stargazers","contributors_url":"https://api.github.com/repos/palantir/plottable/contributors","subscribers_url":"https://api.github.com/repos/palantir/plottable/subscribers","subscription_url":"https://api.github.com/repos/palantir/plottable/subscription","commits_url":"https://api.github.com/repos/palantir/plottable/commits{/sha}","git_commits_url":"https://api.github.com/repos/palantir/plottable/git/commits{/sha}","comments_url":"https://api.github.com/repos/palantir/plottable/comments{/number}","issue_comment_url":"https://api.github.com/repos/palantir/plottable/issues/comments/{number}","contents_url":"https://api.github.com/repos/palantir/plottable/contents/{+path}","compare_url":"https://api.github.com/repos/palantir/plottable/compare/{base}...{head}","merges_url":"https://api.github.com/repos/palantir/plottable/merges","archive_url":"https://api.github.com/repos/palantir/plottable/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/palantir/plottable/downloads","issues_url":"https://api.github.com/repos/palantir/plottable/issues{/number}","pulls_url":"https://api.github.com/repos/palantir/plottable/pulls{/number}","milestones_url":"https://api.github.com/repos/palantir/plottable/milestones{/number}","notifications_url":"https://api.github.com/repos/palantir/plottable/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/palantir/plottable/labels{/name}","releases_url":"https://api.github.com/repos/palantir/plottable/releases{/id}","created_at":"2014-01-22T19:29:19Z","updated_at":"2014-09-17T14:33:36Z","pushed_at":"2014-09-17T19:38:29Z","git_url":"git://github.com/palantir/plottable.git","ssh_url":"git@github.com:palantir/plottable.git","clone_url":"https://github.com/palantir/plottable.git","svn_url":"https://github.com/palantir/plottable","homepage":"plottablejs.org","size":47945,"stargazers_count":111,"watchers_count":111,"language":"JavaScript","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":18,"mirror_url":null,"open_issues_count":151,"forks":18,"open_issues":151,"watchers":111,"default_branch":"develop"}},"_links":{"self":{"href":"https://api.github.com/repos/palantir/plottable/pulls/863"},"html":{"href":"https://github.com/palantir/plottable/pull/863"},"issue":{"href":"https://api.github.com/repos/palantir/plottable/issues/863"},"comments":{"href":"https://api.github.com/repos/palantir/plottable/issues/863/comments"},"review_comments":{"href":"https://api.github.com/repos/palantir/plottable/pulls/863/comments"},"review_comment":{"href":"https://api.github.com/repos/palantir/plottable/pulls/comments/{number}"},"commits":{"href":"https://api.github.com/repos/palantir/plottable/pulls/863/commits"},"statuses":{"href":"https://api.github.com/repos/palantir/plottable/statuses/3dff2db11bb1b81b1d45ba37bf376e9170b21459"}}}}