-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Field overlay view layer for keep in/out zones #915
base: main
Are you sure you want to change the base?
Field overlay view layer for keep in/out zones #915
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.
Looks pretty good functionally. Just a few changes.
@@ -242,6 +242,17 @@ class FieldOverlayRoot extends Component<Props, State> { | |||
<FieldImage2024 /> | |||
</> | |||
)} | |||
|
|||
{layers[ViewLayers.FieldOverlays] && | |||
doc.pathlist.activePath.params.constraints.map((c) => { |
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.
Can this be just the enabled constraints, and additionally filter out the one that's selected (if any)?
uiState.layers[ViewLayers.Waypoints] && | ||
uiState.isNavbarWaypointSelected() && | ||
!this.props.selected | ||
? "rosybrown" |
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.
Not sure I like this color, but that's not a blocker.
src/document/UIData.tsx
Outdated
}, | ||
FieldOverlays: { | ||
index: 6, | ||
name: "FieldOverlays", |
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 naming is too vague and inconsistent with internal naming as well (everything is considered an Overlay).
Clicking a zone in the non-selected layer does not select it. If this is intentional, it's an unexpected break from existing UI norms within the app. If you're going to implement this, keep in mind that a click and drag on the non-selected zone will be hard to catch, since it will be on a different element than the one with the drag handler. |
Region or zone? TrajoptLib uses "keep-in region" and "keep-out region". |
It could be "region". As of current main, Choreo GUI doesn't have a word picked for this. Also worth noting that this new layer includes non-region constraints with display elements (currently just Point At) so we could swerve the question and call the view layer Constraints. |
"Constraints" sounds good to me. |
@Lewis-Seiden what's up with this pr? was there anything that was non-functional or not up to par? |
No description provided.