Add subdomain field support to CLI apps #26
Conversation
There was a problem hiding this comment.
Great job @eliangcs !
I'm requesting changes because I'd like to see the field added to the examples and antiExamples (that runs in the tests).
Other than that I've got a couple of questions on terminology and syntax, basically.
| @@ -101,6 +101,10 @@ module.exports = makeSchema({ | |||
| description: 'Does the value of this field affect the definitions of other fields in the set?', | |||
| type: 'boolean', | |||
| }, | |||
| inputFormat: { | |||
There was a problem hiding this comment.
I'm wondering if we should remove the , from the inputFormat value (because while it simplifies for the backend, there's no real justification for it in this "public-facing" place. It's arguably cleaner if it was just https://{{subdomain}}.yourdomain.com/, and since it applies to subdomain only, should this field name be subdomainFormat instead of inputFormat?)
There was a problem hiding this comment.
I feel that inputFormat could do more than just subdomain, so I've changed the description to be more general.
lib/schemas/FieldSchema.js
Outdated
| @@ -101,6 +101,10 @@ module.exports = makeSchema({ | |||
| description: 'Does the value of this field affect the definitions of other fields in the set?', | |||
| type: 'boolean', | |||
| }, | |||
| inputFormat: { | |||
| description: 'Really only useful for subdomain type. This allows the user to specify a comma-separated string like "https://,{{input}},.yourdomain.com/".', | |||
| type: 'string', | |||
There was a problem hiding this comment.
If we can make this type: 'subdomain', we should be able to add some validation where it then requires inputFormat. Do you think it's an overkill to have it that much explicit?
eliangcs
force-pushed
the
cli-subdomain-field
branch
from
644bc88 to
b25b005
Compare
|
@BrunoBernardino thanks for your review! I've pushed two commits (b25b005 and 11cb76e) with improvements and tests. Please leave feedback. Thanks! |
There was a problem hiding this comment.
Looks great @eliangcs! Thank you so much for the changes, I feel we've got an even better solution now.
I didn't pull this down and run it locally because it seems straightforward enough.
Refer to https://trello.com/c/s8X2tlTY/70-cli-add-support-for-subdomain-format-fields