Add button for adding device panel vertically into view panel#2883
Add button for adding device panel vertically into view panel#2883k311093 wants to merge 1 commit into
Conversation
0405ysj
left a comment
0405ysj
left a comment
There was a problem hiding this comment.
I'm not certain if I'm the right person to review, as I don't have much experience on Angular.
| : -item.w; | ||
|
|
||
| item.x = lastMaxX + item.w * 2 <= this.cols ? lastMaxX + item.w : 0; | ||
| item.y = lastMaxX + item.h * 2 <= this.cols ? lastMaxY : lastMaxY + item.h; |
There was a problem hiding this comment.
lastMaxY instead of lastMaxX?
There was a problem hiding this comment.
lastMaxX was right because the statement was for checking new item exceeds view panel width, but item.h was not right. Changed to item.w.
| const lastMaxY = | ||
| layout.length !== 0 | ||
| ? Math.max(...layout.map(obj => obj.y + obj.h)) | ||
| : 0; |
There was a problem hiding this comment.
I'm not certain what lastMaxX or lastMaxY are representing.
My naive thought around x,y,w,h is:
- left: x
- right: x+w
- top: y
- bottom: y+h
However I can see y+h & y is used together for calculating lastMaxY.
There was a problem hiding this comment.
lastMaxY for upper if statement was for Y coordinate for new item.
For lastMaxY in the else block it is for the last item's coordination for calculate the place of new item.
Changed names for describe it's usage more.
| ? Math.max(...lastRowLayout.map(obj => obj.x)) | ||
| : -item.w; | ||
|
|
||
| item.x = lastMaxX + item.w * 2 <= this.cols ? lastMaxX + item.w : 0; |
There was a problem hiding this comment.
As following comment of the above comment, I don't understand how the calculation item.w * 2 is coming from.
I can only make a rough guess that it could be implemented with only assuming homogeneous(e.g. 2 identical CF phone instances) scenario. But probably heterogeneous scenario(e.g. 1 CF phone + 1 CF tablet) should be considered too?
URL for above comment: https://github.com/google/android-cuttlefish/pull/2883/changes#r3635124511
There was a problem hiding this comment.
* 2 is coming from zoom ratio (0.5) of initially added device.
Changed to use the constant to calculate it.
|
|
||
| const updatedLayout = Array.from(layoutById, ([, value]) => value); | ||
|
|
||
| if (this.displaysService.addVertically$.value) { |
There was a problem hiding this comment.
Why only vertical display needs sort?
aea5e03 to
4f270eb
Compare
In the multiple device scanerio with landscape device screen, stacking device panel horizontally is not a good experience to use. (it need to scroll page right side to see the device). Add a toggle button to adding device panel vertically to support this case. b/534778982
| : -item.w; | ||
|
|
||
| item.x = lastItemX + item.w * (1.0 / this.defaultDisplayZoom) <= this.cols ? lastItemX + item.w : 0; | ||
| item.y = lastItemX + item.w * (1.0 / this.defaultDisplayZoom) <= this.cols ? lastItemY : lastItemY + item.h; |
There was a problem hiding this comment.
For better visibility, please consider to remove ternary operator with complicated/duplicated condition.
Or, what about the code looks like below?
maxBottom= ...
if (this.displaysService.addVertically$.value) {
stackVertical = true
} else {
maxRight = ...
stackVertical = maxRight + item.w * (1.0 / this.defaultDisplayZoom) <= this.col
}
if (stackVertical) {
item.x = 0
item.y = maxBottom + item.h
} else {
item.x = maxRight + item.w
item.y = maxBottom
}
3 participants
In the multiple device scanerio with landscape device screen, stacking device panel horizontally is not a good experience to use. (it need to scroll page right side to see the device).
Add a toggle button to adding device panel vertically to support this case.
b/534778982