-
Notifications
You must be signed in to change notification settings - Fork 35
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
Nadro/adhoc/center rectangle #4480
base: main
Are you sure you want to change the base?
Conversation
…r rectangle expressions
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
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.
Nice! This works well for me while testing locally. I only had nits
Oh wtf my VS Code extension didn't save the comments, so annoying |
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.
One addition I'd call out is that if you want this to appear in the command bar as a part of this PR, you should add the center rectangle tool to the modelingCommandConfig.ts
, but that's not critical just another way to equip the tool. Great job!
: 'none', | ||
}, | ||
}), | ||
icon: 'arc', |
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.
(comment): This looks like you might have been testing out if it had any effect, which it doesn't. So maybe we need a TODO
to make the dropdown items support icons; I can make a Center Rectangle icon coming up soon.
;((pipeExpression.body[1] as CallExpression) | ||
.arguments[0] as ArrayExpression) = createArrayExpression([ |
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.
(nit): Should this make use of your new cool type narrowing functions as well?
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.
(comment): I didn't know about this syntax, super cool
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.
(comment): Kurt's done some cool work to make this stuff less boilerplate-filled, and I'd love your ideas as you touch code like this on how to take it further, because it still feels repetitive and "getting your hands dirty", na mean?
@@ -215,6 +217,7 @@ export function Toolbar({ | |||
} | |||
const itemConfig = maybeIconConfig | |||
|
|||
// A single button |
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.
(comment):
// A single button
...followed by like 40 lines of JSX lol
closes #4402
Implements
Center Rectangle
in the toolbar and updated it with the new stateRectangle
but forCenter Rectangle
createLiteral
definition since it didn't actually use the typeLiteralValue
Gotchas
startProfileAt
but once they click into the second state for dragging the width and height it will be corrected since there is dimension to the box. It effectively says the startProfile at is the center with width/height of 0.Future improvements