-
Notifications
You must be signed in to change notification settings - Fork 60
Add a utility to lay out actors in a grid #492
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the concept of a layout utility, but there are some additional features we should include. Can we back up and have a design session?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @eanders-ms on a design PR for this.
I'm surprised to hear this, I thought tables were pretty self-explanatory, with pre-defined expectations. Is this PR missing features that are so badly needed that shipping without the feature is worse than not having the layout at all? |
A grid layout utility is worthwhile addition. If it is going to be an SDK component, it just needs a design review. Some ideas I'd like to cover:
MRTK has something like this. See: https://microsoft.github.io/MixedRealityToolkit-Unity/Documentation/README_ObjectCollection.html |
@eanders-ms These other modes do seem useful, but their lack shouldn't disqualify this PR I think. Perhaps I could simply rename the utility |
You're suggesting separate classes for different surface types? |
packages/sdk/src/util/gridLayout.ts
Outdated
colWidths[cell.column], rowHeights[cell.row] | ||
); | ||
|
||
cell.contents.transform.local.position = gridAlign.add(cellPosition).add(cellAlign); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cell.contents.transform.local.position [](start = 3, length = 38)
Would be nice: Option to interpolate this change. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Easy to add, done! #Resolved
I've added a design document, and made some naming tweaks. Review? |
} | ||
} | ||
appearance: { meshId: ball.id } | ||
}}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
semicolon :) #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still mid-statement, can't put a semi-colon here.
In reply to: 385870701 [](ancestors = 385870701)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops I misread it. #Closed
) { } | ||
|
||
/** The number of columns in this grid. */ | ||
public columnCount() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
columnCount [](start = 8, length = 11)
I suggest making this and the rest getters. Or, prefix the method name with get
--> getColumnCount
#Closed
* The width of a particular column. | ||
* @param i The column index. | ||
*/ | ||
public columnWidth(i: number) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
columnWidth [](start = 8, length = 11)
Suggest naming this getColumnWidth
#Closed
contents: label | ||
}); | ||
} | ||
buttonGrid.applyLayout(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
applyLayout [](start = 13, length = 11)
How about making this animate? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🕐
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I must've implemented this half a dozen times by now. Finally I have a grid layout that's reusable!