feat: create and edit release plan template milestones

[daveleek]

Adding and editing milestones and their names

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
unleash-monorepo-frontend	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 19, 2024 0:26am

1 Skipped Deployment

Name	Status	Preview	Comments	Updated (UTC)
unleash-docs	⬜️ Ignored (Inspect)	Visit Preview		Nov 19, 2024 0:26am

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

Package	Version	Score	Details

Scanned Files

[nunogois]

Why isn't this a part of the above block, since the condition is the same?

[daveleek]

Argh, I moved things around and forgot to remove the duplicate condition 🙄

[nunogois]

I think you could have added the onClick handler on the parent component, catching any click in the children, instead of duplicating it.

[daveleek]

Parent is the same for this and the other section which is only shown during edit mode, and would have a weird effect if you click inside. I'll have a think if I can introduce a parent just for these two while doing add strategies

[nunogois]

Try not to rely on the index here. It's going to be especially troublesome if we decide to allow the user to sort / change the order of the milestones.

Ideally you would have unique ids you could use for each milestone, however those are generated only in the backend. My suggestion is creating temporary uuids in the frontend for state management and omitting them from the final payload.

[nunogois]

I still think relying on indexes here will bite you down the line, but if you're willing to accept that then feel free to proceed for now.

[daveleek]

I'm going to have a look at that together with adding strategies!

[nunogois]

Why is this a payload instead of just IReleasePlanMilestone?

[daveleek]

I'll have a look at the types when doing add strategies :)

[nunogois]

I believe we should take another look at some of these points before merging:

• Duplicated code between CreateReleasePlanTemplate and EditReleasePlanTemplate
• State management
• Relying on index for milestone management

[nunogois]

I still think you could have potentially merged the edit and create components into a single one, with a simple const isEdit = Boolean(templateId), but this is probably fine for now, and has much less duplicated code than before, so good job 👍

[nunogois]

Why not use IReleasePlanMilestonePayload | undefined here? Should be more explicit than -1 and indexes.

[daveleek]

This one is for adding strategies to the milestone. Current milestone (should probably be named ...index) gets passed to the add strategy form. If we're adding something to that object (like we do with names) it should be bubbled up no?

[nunogois]

This is definitely better now, especially the create / edit component part.

We already discussed the points I'm worried about, so I'll leave you a non-blocking approval and leave the decision up to you. Some of the issues I'm concerned about you'll eventually be forced to tackle, even if it's later down the line.