Add platform field fixes #1048#1130
Conversation
|
I did this first since it seems like a dependency for #747. |
jaredlockhart
left a comment
jaredlockhart
left a comment
There was a problem hiding this comment.
@glasserc Fantastic this is perfect, thank you! I'm trying to get into the habit of including screenshots of PRs that change UI stuff, so for this it would've been nice to see 1) the new form fields 2) the new info on the detail page. You don't have to do it here, but for future PRs :)
|
|
|
To be honest I'm not 100% convinced about having any fields before the population match description, it almost reads like part of the last field (in this case, Platforms). |
|
@glasserc Oh you mean on the detail page, there's no header for 'client matching' so it just looks like a continuation of the last field? You could just add a little title like |
|
Yes, I think that's better..
|
|
@glasserc Yep looks good! Can you rebase this on master and regenerate the migration so that it's number 40 |
Per #971, these fields should go *above* the current client_matching field.
|
OK, done. I also added factories for the locales/countries. The new migration is numbered 40, which will conflict with #1134, but I'm happy to regenerate the migrations again if that helps. |
Thanks @jaredkerim for the suggestion.
|
OK, the locales/countries factories are not fully baked, so I'll make that it into another PR. This one will just have to be platforms. |
2 participants
No description provided.