Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2916983002:
shelf: Allow directly dragging items from main shelf to overflow. (Closed)
|
Created:
3 years, 6 months ago by sammiequon Modified:
3 years, 6 months ago Reviewers:
msw CC:
chromium-reviews, kalyank, sadrul Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionshelf: Allow directly dragging items from main shelf to overflow.
Previously we would have to drag the item up off the main shelf and then down onto the overflow shelf. This would cause extra difficutly if the items were vertically aligned. Also allows dragging items from the overflow down onto the main shelf.
TEST=ash_unittest --gtest_filter="ShelfViewTest.*"
BUG=711817
Review-Url: https://codereview.chromium.org/2916983002
Cr-Commit-Position: refs/heads/master@{#477432}
Committed: https://chromium.googlesource.com/chromium/src/+/bbd450e340cae1277f2f907226735cb7154270b3
Patch Set 1 #
Total comments: 8
Patch Set 2 : Fixed patch set 1 errors. #
Messages
Total messages: 21 (14 generated)
The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:1) has been deleted
The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run
Description was changed from ========== shelf: Allow directly dragging items from main shelf to overflow. Previously we would have to drag the item up off the main shelf and then down onto the overflow shelf. This would cause extra difficutly if the items were vertically aligned. Also allows dragging items from the overflow down onto the main shelf. TEST=ash_unittest --gtest_filter="ShelfViewTest.*" BUG=711817 ========== to ========== shelf: Allow directly dragging items from main shelf to overflow. Previously we would have to drag the item up off the main shelf and then down onto the overflow shelf. This would cause extra difficutly if the items were vertically aligned. Also allows dragging items from the overflow down onto the main shelf. TEST=ash_unittest --gtest_filter="ShelfViewTest.*" BUG=711817 ==========
sammiequon@chromium.org changed reviewers: + msw@chromium.org
msw@ - Please take a look. Thanks!
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Some minor comments https://codereview.chromium.org/2916983002/diff/20001/ash/shelf/shelf_view.cc File ash/shelf/shelf_view.cc (right): https://codereview.chromium.org/2916983002/diff/20001/ash/shelf/shelf_view.cc... ash/shelf/shelf_view.cc:1137: // Determine if we should enter the ripped off state. We should enter if: nit: "Mark the item as dragged off the shelf if the drag distance exceeds kRipOffDistance, or if it's dragged between the main and overflow shelf." https://codereview.chromium.org/2916983002/diff/20001/ash/shelf/shelf_view.cc... ash/shelf/shelf_view.cc:1144: bool enableRipOff = delta > kRipOffDistance; Use the unix_hacker naming convention in Chrome's C++ code |drag_off_shelf|, or just use the |dragged_off_shelf_| member, or just inline the conditionals into the if statement. https://codereview.chromium.org/2916983002/diff/20001/ash/shelf/shelf_view.cc... ash/shelf/shelf_view.cc:1146: enableRipOff || nit: if you keep separate statements like this, then use |= instead of = ... || https://codereview.chromium.org/2916983002/diff/20001/ash/shelf/shelf_view_un... File ash/shelf/shelf_view_unittest.cc (right): https://codereview.chromium.org/2916983002/diff/20001/ash/shelf/shelf_view_un... ash/shelf/shelf_view_unittest.cc:1259: // Test that certain drag movements will rip or not rip off the app from the nit: "Test what drag movements will rip an item off the shelf."
https://codereview.chromium.org/2916983002/diff/20001/ash/shelf/shelf_view.cc File ash/shelf/shelf_view.cc (right): https://codereview.chromium.org/2916983002/diff/20001/ash/shelf/shelf_view.cc... ash/shelf/shelf_view.cc:1137: // Determine if we should enter the ripped off state. We should enter if: On 2017/06/06 00:27:23, msw wrote: > nit: "Mark the item as dragged off the shelf if the drag distance exceeds > kRipOffDistance, or if it's dragged between the main and overflow shelf." Done. https://codereview.chromium.org/2916983002/diff/20001/ash/shelf/shelf_view.cc... ash/shelf/shelf_view.cc:1144: bool enableRipOff = delta > kRipOffDistance; On 2017/06/06 00:27:23, msw wrote: > Use the unix_hacker naming convention in Chrome's C++ code |drag_off_shelf|, or > just use the |dragged_off_shelf_| member, or just inline the conditionals into > the if statement. Done. https://codereview.chromium.org/2916983002/diff/20001/ash/shelf/shelf_view.cc... ash/shelf/shelf_view.cc:1146: enableRipOff || On 2017/06/06 00:27:23, msw wrote: > nit: if you keep separate statements like this, then use |= instead of = ... || Done. https://codereview.chromium.org/2916983002/diff/20001/ash/shelf/shelf_view_un... File ash/shelf/shelf_view_unittest.cc (right): https://codereview.chromium.org/2916983002/diff/20001/ash/shelf/shelf_view_un... ash/shelf/shelf_view_unittest.cc:1259: // Test that certain drag movements will rip or not rip off the app from the On 2017/06/06 00:27:23, msw wrote: > nit: "Test what drag movements will rip an item off the shelf." Done.
lgtm
On 2017/06/06 18:01:19, msw wrote: > lgtm Thanks!
The CQ bit was checked by sammiequon@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496785384292310,
"parent_rev": "e34bc57b5f2eae6c98e53c7e70482be51af5a2ea", "commit_rev":
"bbd450e340cae1277f2f907226735cb7154270b3"}
Message was sent while issue was closed.
Description was changed from ========== shelf: Allow directly dragging items from main shelf to overflow. Previously we would have to drag the item up off the main shelf and then down onto the overflow shelf. This would cause extra difficutly if the items were vertically aligned. Also allows dragging items from the overflow down onto the main shelf. TEST=ash_unittest --gtest_filter="ShelfViewTest.*" BUG=711817 ========== to ========== shelf: Allow directly dragging items from main shelf to overflow. Previously we would have to drag the item up off the main shelf and then down onto the overflow shelf. This would cause extra difficutly if the items were vertically aligned. Also allows dragging items from the overflow down onto the main shelf. TEST=ash_unittest --gtest_filter="ShelfViewTest.*" BUG=711817 Review-Url: https://codereview.chromium.org/2916983002 Cr-Commit-Position: refs/heads/master@{#477432} Committed: https://chromium.googlesource.com/chromium/src/+/bbd450e340cae1277f2f90722673... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as https://chromium.googlesource.com/chromium/src/+/bbd450e340cae1277f2f90722673... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
