- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 751
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
feat: implement dialogs for changerequest milestone handling and removing release plans #9240
feat: implement dialogs for changerequest milestone handling and removing release plans #9240
Conversation
daveleek
commented
Feb 6, 2025
…ving release plans
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
<ReleasePlanAddChangeRequestDialog | ||
action='startMilestone' | ||
environmentId={environment} | ||
featureId={featureName} | ||
onClosing={() => { | ||
setPlanForChangeRequestStartMilestoneDialog(undefined); | ||
setMilestoneForChangeRequestDialog(undefined); | ||
}} | ||
projectId={projectId} | ||
releasePlan={planForChangeRequestStartMilestoneDialog} | ||
milestone={milestoneForChangeRequestDialog} | ||
/> |
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 set two up here so I wouldn't have to let the action be nullable & have another react state for it. I could change the logic in the dialog to looking at milestone being set, only release plan being set, or only template being set though. WDYT @nunogois?
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.
Uh sure. Honestly I would have expected separate dialog components for each action, but if this works and you're happy I don't think it matters much 🤷
…nge-request-dialogs
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files |
|
||
const StyledBoldSpan = styled('span')(({ theme }) => ({ | ||
fontWeight: theme.typography.fontWeightBold, | ||
})); | ||
|
||
interface IReleasePlanAddChangeRequestDialogProps { | ||
action: 'addReleasePlan' | 'deleteReleasePlan' | 'startMilestone'; |
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.
Is this needed now that we split them 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.
Strange, I removed these. why doesn't that show up?
releaseTemplate?: IReleasePlanTemplate | undefined; | ||
releasePlan?: IReleasePlan | undefined; | ||
milestone?: IReleasePlanMilestone | undefined; | ||
environmentActive?: boolean; |
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.
Seems like these 2 are unused. One of the advantages of splitting them up into separate components is that each one can request exactly what they need.
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 really weird, I removed these
@@ -45,7 +62,7 @@ export const ReleasePlanAddChangeRequestDialog = ({ | |||
return ( | |||
<Dialogue | |||
title='Request changes' | |||
open={Boolean(releaseTemplate)} | |||
open={Boolean(releaseTemplate) || Boolean(releasePlan)} |
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 a huge fan of this ambiguity. What is this dialog doing that it shows up if one of these is truthy? I also prefer to handle the open state explicitly instead of relying on other state variables, but that's mostly a personal preference.
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 also gone, very strange
projectId: string; | ||
featureId: string; | ||
environmentId: string; | ||
releaseTemplate: IReleasePlanTemplate | undefined; | ||
releaseTemplate?: IReleasePlanTemplate | undefined; |
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.
No need for the | undefined
with the ?
.
text: 'Added to draft', | ||
}); | ||
onClosing(); | ||
}; |
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.
IMO the caller should own this. I think dialogs should only care about displaying the required information and emitting the "confirmation" callback to the parent, which then handles it. Something like onConfirm
.
const [ | ||
milestoneForChangeRequestDialog, | ||
setMilestoneForChangeRequestDialog, | ||
] = useState<IReleasePlanMilestone | undefined>(); |
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.
You can use something like selectedReleasePlan
and share that state across multiple dialogs, since only one of them will be open at a time. I'm also pretty sure you don't need to specify | undefined
here, it's implicit.
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.
What's preventing us from making these dialogs work the same way as the existing ReleasePlanRemoveDialog
?
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.
since only one of them will be open at a time
We moved the "am I open" part inside the dialog, so this follows that pattern. And to check if it's open - it checks to see if the provided property is defined (or boolean). So if we share, then both delete and start milestone will open in some sequence.
We could move to having an isOpen for them, but then that would be 3 isOpens instead.
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.
but then that would be 3 isOpens instead.
That feels more intuitive to me.