Bug-1995794: Disable retrigger dropdown when there are no runs - #1087
Bug-1995794: Disable retrigger dropdown when there are no runs#1087moijes12 wants to merge 2 commits into
Conversation
✅ Deploy Preview for mozilla-perfcompare ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
bfccec1 to
d8d2493
Compare
beatrice-acasandrei
left a comment
beatrice-acasandrei
left a comment
There was a problem hiding this comment.
In my opinion it would be helpful to gray out the dropdown text to clearly indicate that the field is disabled. @esanuandra @kala-moz What do you think?
There was a problem hiding this comment.
Consider disabling "Confirm" when both are disabled:
| <Button | |
| type='submit' | |
| disabled={!props.hasBaseJobs && !props.hasNewJobs} | |
| > | |
| {retriggerStrings.submitButton} | |
| </Button> |
| expect(new MockedHooks().triggerHook).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| async function openRetriggerConfigModal( |
There was a problem hiding this comment.
Test helper should be at the top of the describe block before any test cases, not in the middle
| labelId={`${prefix}-retrigger-count-label`} | ||
| name={`${prefix}-retrigger-count`} | ||
| defaultValue={5} | ||
| defaultValue={disabled ? 0 : 5} |
There was a problem hiding this comment.
Could you add a brief explanatory comment here? It’s worth noting that using defaultValue is safe in this context because the modal unmounts when closed, ensuring the state re-initializes on each open.
| name={`${prefix}-retrigger-count`} | ||
| defaultValue={5} | ||
| defaultValue={disabled ? 0 : 5} | ||
| disabled={disabled} |
There was a problem hiding this comment.
FormControl automatically passes disabled down to its children through React context. So passing disabled explicitly to Select too is unnecessary. It's not a bug — it just creates extra noise and could confuse a reader into thinking it's needed for a reason.
| disabled={disabled} |
Disable the Base and New retrigger count selects independently when there are no retriggerable job ids for that side, and default a disabled select to 0 so it does not look like jobs will start.
Drop the redundant Select disabled prop, document why defaultValue is safe, move the retrigger test helper to the top of the describe block, and disable submit when neither base nor new can be retriggered.
3d01970 to
bea514f
Compare
3 participants
Disable the Base and New retrigger count selects independently when there are no retriggerable job ids for that side, and default a disabled select to 0 so it does not look like jobs will start.
@beatrice-acasandrei @kala-moz