Support constraints! #76
Conversation
| minItems: PropTypes.number, | ||
| uniqueItems: PropTypes.bool | ||
| }) | ||
| }; |
There was a problem hiding this comment.
This class's responsibility is to render the validation constraints for arrays, and thus should be named accordingly.
Need one space in UI (Carriers > Returns a list of integrations available for registration)
There was a problem hiding this comment.
Do you want all the *Property classes to be renamed to *Constraints (I don't feel strongly either way)?
As for the space, I'd add some CSS.
There was a problem hiding this comment.
Yes, they should either have Constraint in the name, or are all under a Constraints directory. That makes it easy to follow.
| minimum: PropTypes.number, | ||
| multipleOf: PropTypes.number | ||
| }) | ||
| }; |
There was a problem hiding this comment.
It looks like the construction of the constraints in those new constraints components could be extracted out, which would make the logic a bit easier to test (and those components could potentially go away??? We can then have a shared ConstraintComponent that requires {validations, validationType}. What do you reckon @brendo?
There was a problem hiding this comment.
I don't agree sorry. I considered having one component be responsible for constraints, but ultimately it would still have switch logic in it to only apply certain constraints to certain components.
Having small (tiny even!) components that just focus on all things Numeric or String made for really easy testing and if we want to add more constraints, we'd only have to touch a small part of the codebase!
I'm happy to dumb down the Property constraints proptype (from shape to object) and just leave the smaller components deal with the actual specifics if you think that'd reduce duplication?
There was a problem hiding this comment.
I totally agreed with you about the approach of having multiple small components.
My point was that all those constraints UI components share the same pattern:
- Construct the
validationsarray - Render the validations array, e.g.
<span>
{validations.map(constraint =>
<span key={constraint} className={constraintName}>{constraint}</span>
)}
</span>
So they only differ in the way validations are constructed. This part, however, has nothing to do with UI.
If we move the validation logic out (as we try to keep the UI components really lean), to seperate functions, then there are no difference between those UI constraint components. React components offer PropTypes.shape, which is a nice way to specify the signature (input) of the UI component; however we could achieve something like that with function signature and typescript (down the track).
There was a problem hiding this comment.
Ah got you, I have an idea, let me come back to you...
| @@ -0,0 +1,56 @@ | |||
| import { VALIDATION_KEYWORDS, hasConstraints, getConstraints } from '../../src/parser/open-api/constraintsParser'; | |||
There was a problem hiding this comment.
this test should be under test/parser/open-api
Support constraints!
Support constraints!
There's a few changes in here, but should be relatively straightforward to understand.
From reading the JSON Schema specification, most of the constraints only apply to certain types. This made for a simple (if not verbose) implementation, where the
Propertycomponent delegates most of the work to sub components that are responsible for rendering the constraints.This change comes with zero styling, that'll be part of #38.
In addition this fixes a couple of other issues:
0orfalse(strict compare)Array of stringto be simplystring[]