feat: add copy increment button action#4219
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR implements a "Copy +N" feature: it discovers incrementable fields in a control's JSON, exposes options via TRPC, applies numeric/text increments to selected fields when copying controls, and adds a frontend modal and button workflow to configure and execute incremented copies. ChangesCopy +N Control Increment Feature
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4037edc9-8c62-4513-a02b-0ecfa7dbf2bc
📒 Files selected for processing (5)
companion/lib/Controls/ControlIncrementUtil.tscompanion/lib/Controls/ControlsTrpcRouter.tscompanion/test/Controls/ControlIncrementUtil.test.tswebui/src/Buttons/ButtonGridActions.tsxwebui/src/scss/_button-grid.scss
There was a problem hiding this comment.
Thanks for the PR, I like the idea behind this!
From a UX side:
- I feel like the placement of the button and calling it '+1' is not the most clear on what it does
- This should be added to the context menu that was recently added to the grid
- There needs to be a confirm step before replacing an existing button; I think it is too easy to forget it is still active and to accidentally replace a button you dont mean to.
- I think it would be good to have a quick explainer block of text at the top to explain what this does
- I would prefer the
Local variable 1 (me)to beLocal varible: meor perhaps it should be$(local:me)to show it in a familar way? - It looks like it has picked up colour feedback values, which is weird. It is hard to tell what these feedback values are.
- As this is doing replacements in strings, it would be good to show a preview of what those values will become. Should it be incrementing every number or just the last found number?
I am wondering whether this should support adjusting any feedback/action/style option, or if it should be limited to just local variables. While I can see value in it being able to operate on anything, I think its going to either add a lot of complexity to be able to accurately filter and label what it is showing.
| input.incrementBy | ||
| ) | ||
|
|
||
| // Delete the control at the destination |
There was a problem hiding this comment.
Here onwards could probably be replaced with controlsController.importControl(toLocation, controlJson, true) and check the return was a string for success.
There was a problem hiding this comment.
I feel like some of this css shouldnt be necessary, it would be better to use existing util when possible. For the grid/layout stuff that should be possible, probably not for the badge
| return key.replace(/^opt:/, '').replace(/_/g, ' ') | ||
| } | ||
|
|
||
| function isExpressionValueObject(value: unknown): value is { value: unknown; isExpression: boolean } { |
There was a problem hiding this comment.
there is already a util for this
There was a problem hiding this comment.
I'm not liking how generically this is searching. As some of the label generation shows, this loses all the context on what everything is, which is resulting in the functions for generating the label based on the paths, and leaves parts of the labels as abstract/internal names that don't mean anything to the user.
And I worry that as we adjust terminology or add functionality elsewhere, it will be very easy to forget about the label generation here resulting in them diverging
In my modules, a bunch of the options fields are named very poorly as they were chosen years ago but they arent user facing so there has been no reason to go through the effort of renaming them.

Summary
Verification
git diff --checkyarn test --run companion/test/Controls/ControlIncrementUtil.test.tsyarn check-typeseslint,prettier --checkSummary by CodeRabbit
New Features
Tests