-
Notifications
You must be signed in to change notification settings - Fork 198
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(nimbus): Add option to add targeting criteria on the Audience page #12384
feat(nimbus): Add option to add targeting criteria on the Audience page #12384
Conversation
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.
Overall looks good, main issue is that I think the request URL should point to experimenter?
Also, tests?
Co-authored-by: Mike Williams <[email protected]>
Thank you @mikewilli for the review, updated the URL, also for the tests we are not testing each page UI for htmx, but this will cover as part of the integration tests |
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.
Couple comments, but nothing too major so won't block on them.
TARGETING_CRITERIA_REQUEST = ( | ||
"https://github.com/mozilla/experimenter/issues/new" | ||
"?template=targeting_request_template.yml" | ||
"&title=Targeting%20criteria%20request" |
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.
Does this title override the default title you put in the template? If so, I'd leave it out of the URL (and update the template's title instead if needed).
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 need to do this testing once this gets landed, I just followed the instructions :)
name: Targeting Criteria Request | ||
description: Request new advanced targeting logic for an experiment. | ||
title: "[Targeting] " | ||
labels: [targeting, enhancement] |
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.
Just confirming that we have both of these labels (if not, want to create them?).
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 will remove this
Co-authored-by: Mike Williams <[email protected]>
Because
This commit
Fixes #12096

Screen.Recording.2025-03-26.at.7.15.04.PM.mov