Skip to content

Conversation

@zackcl
Copy link
Collaborator

@zackcl zackcl commented Sep 12, 2025

Resolves #2631

@zackcl zackcl requested review from bcb37 and danoswaltCL September 12, 2025 11:21
@zackcl zackcl self-assigned this Sep 12, 2025
@zackcl zackcl marked this pull request as ready for review September 26, 2025 09:34
@zackcl zackcl changed the title [WIP] Implement Upsert Metric Modal for Experiments Implement Upsert Metric Modal for Experiments Sep 26, 2025
@danoswaltCL
Copy link
Collaborator

Mooclet rewardsMetricKey from other experiments in that context are showing up in the dropdown, should be hidden unless it's that experiment's metric key.

image

@danoswaltCL
Copy link
Collaborator

Minor thing but maybe some extra padding / white space to prevent it scrolling right up to the top and bottom of the content container? Not sure why but it feels like it should have some buffer:

image

@danoswaltCL
Copy link
Collaborator

danoswaltCL commented Oct 31, 2025

Sometimes it is letting me submit an invalid metric key name, when the metric is null

image

And other times it is throwing an error, which is good but the error says it's blank, which is not quite the reason, it's just invalid.

fail:
image
image

@danoswaltCL
Copy link
Collaborator

I have not looked at this very deeply yet, just got started on Friday afternoon. I already like this a lot more than the old modal, looks nice.

We should have some research team members use this and see what notes they have. I think they'll be relieved to see it updated, this makes more sense to me at least.

@danoswaltCL
Copy link
Collaborator

the "description" is missing from submitted request. noticed when i hit edit and it wasn't populated. is this description seen in the UI anywhere? i guess maybe it will show on the data tab?

image

@danoswaltCL
Copy link
Collaborator

small thing, when editing I can submit without making changes. in other edit modals I think we usually don't allow submit until user enters something.

@danoswaltCL
Copy link
Collaborator

danoswaltCL commented Nov 3, 2025

I don't think we have employed 'speedbumps' in newer edit modals for 'are you sure you want to discard changes?' I know we have that in some places in old design. I think we may want that for certain forms if the user is entering enough info to be frustrated by an accidental exit.

Which is to say, maybe this modal ought to have a speedbump, thoughts @zackcl @amurphy-cl @bcb37

@danoswaltCL
Copy link
Collaborator

I'd like to ask other research folks if the display of this information works for them. Maybe it does, I know it's basically what we have been doing all along.

It's always looked like text soup to me. And the way the words line up particularly in a group-metrics row makes my brain automatically want to read each row left-to-right, where each row is an entity and the columns are properties. which isn't the way this reads obviously, a little jarring even knowing how this reads in columns.

I don't know if we can go wrong labelling the crap out of everything metric related, metrics are complicated. The table deserves the same level of friendliness as the new modal. our #upgrade3.0 adoptees will appreciate it. @zackcl @amurphy-cl @bcb37

image

Copy link
Collaborator

@danoswaltCL danoswaltCL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left comments on some things that are missing, and some design questions for the team.

@zackcl
Copy link
Collaborator Author

zackcl commented Nov 24, 2025

Closing due to a new PR #2756 that includes all the changes made in this PR.

@zackcl zackcl closed this Nov 24, 2025
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.

3 participants