Skip to content

Conversation

@zackcl
Copy link
Collaborator

@zackcl zackcl commented Nov 21, 2025

Resolves #2631

@danoswaltCL I've opened this new PR since the old PR #2648 had too many conflicts with the latest experiment-design-refresh branch. I've applied all the changes in #2648 in this PR and then made these further changes:

  • Filtered out all reward metric options in the Metric ID field. (Needs confirmation - let me know if the reward metric key of the current experiment should remain visible.) (fixes the comment)
  • Disallowed editing and deleting reward metrics.
  • Added validation requiring metric class, key, and ID selection from the provided options, with cascading field clearing. (fixes the comment)
  • Prevented the Save button from being incorrectly enabled in edit mode for categorical metrics. (fixes the comment)
  • Applied the appTrimInput directive to all input fields in UpsertMetricModalComponent.

Here are some of your comments that are unresolved and needs discussion:

  • Need extra top/bottom paddings for the inner scrollable content in the modal (comment) Dan: disregard
  • The "Description" field is missing from the submitted request. Do we even store this in the DB? If not, should we remove this field from the modal? (comment) Dan: this can be removed
  • Maybe this modal needs a speedbump (comment) Dan: disregard, not needed!
  • Maybe we should improve the display of the Metrics table ([comment] (Implement Upsert Metric Modal for Experiments #2648 (comment))) Dan: this should be a new story for later, @zackcl maybe some draft design ticket?

@zackcl zackcl requested review from bcb37 and danoswaltCL November 21, 2025 19:25
@zackcl zackcl self-assigned this Nov 21, 2025
@zackcl zackcl changed the title [WIP] Implement Upsert Metric Modal for Experiments v2 Implement Upsert Metric Modal for Experiments v2 Nov 24, 2025
@zackcl zackcl marked this pull request as ready for review November 24, 2025 15:42
@zackcl zackcl marked this pull request as draft December 2, 2025 18:39
@zackcl zackcl marked this pull request as ready for review December 2, 2025 20:21
@zackcl zackcl marked this pull request as draft December 2, 2025 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants