fix #3749 fix #3750: audience form with saved data #4071
Conversation
lmorchard
force-pushed
the
3749-audience-dummy-form
branch
from
27f8963
to
da87bf2
Compare
lmorchard
force-pushed
the
3749-audience-dummy-form
branch
from
da87bf2
to
155f818
Compare
lmorchard
changed the title
| </Form.Row> | ||
| </Form.Group> | ||
|
|
||
| <Form.Group className="bg-light p-3"> |
There was a problem hiding this comment.
Nit: I think p-4 is a bit closer to Figma, as well as removing className="mb-3" on InputGroup on L99 and L126.
| <Form.Control | ||
| placeholder="0.00" | ||
| aria-describedby="clientsPercent-unit" | ||
| type="text" |
There was a problem hiding this comment.
I think it could be nice to have the 4 inputs that only accept integers as type="number" for the up/down arrow selection and slightly better validation onblur.
I'm guessing some of these should have a maximum allowed value too - we could create a quick follow up ticket for that and throw it into the maintenance fix version. Since max values and the links mentioned in another comment will come from collab with someone (probably Ana), might make sense to make one task for both?
There was a problem hiding this comment.
Planning on doing this when I add validation in the next issue (#3782)
| </div> | ||
| ); | ||
|
|
||
| export const MOCK_EXPERIMENT = mockExperimentQuery("demo-slug", {})!.data!; |
There was a problem hiding this comment.
I think this can be minorly simplified to mockExperimentQuery("demo-slug").data!
There was a problem hiding this comment.
Yeah, this is mainly left over from when I customized the experiment, but ended up gradually stripping all that out. This can go too
| getConfig_nimbusConfig_channels, | ||
| } from "../../types/getConfig"; | ||
|
|
||
| // TODO: find this doco URL |
There was a problem hiding this comment.
I think we have a couple of these "find link" TODOs now, one in Audience and one in Branches. We should probably have a follow up ticket made to find/fix these.
There was a problem hiding this comment.
Filed as "Find+fix all the TODO "find link" documentation URLs #4101".
| <Form.Row> | ||
| <Form.Group as={Col} controlId="channel" md={8} lg={8}> | ||
| <Form.Label>Channel</Form.Label> | ||
| <Select |
There was a problem hiding this comment.
This multiselect looks great! Not sure why "Select..." is a slightly different color but we can fine-tune stuff later (definitely not worth looking into if it's not quick, I mostly wanted to say the multi select is cool :D)
Because: - we want to manage audience attributes for an experiment This commit: - adds react-select as a dependency - introduces FormAudience component to manage audience attributes - displays saved audience data and uses configuration for valid choices
lmorchard
force-pushed
the
3749-audience-dummy-form
branch
from
155f818
to
b875c9c
Compare
|
Fixing a couple nits. Then, going to merge in order to move on to #3782 for form validation and wiring up to GQL mutation |
Because:
This commit:
adds react-select as a dependency
introduces FormAudience component to manage audience attributes
displays saved audience data and uses configuration for valid choices