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
fix(Date Value Validators): Propagate valueRequiredValidator to child component (DSP-1188) #251
Conversation
@tobiasschweizer @flavens efdd557 Fixes the issue for the date value component. We know we will have this issue for other complex value components as well. Should I fix them in this PR or open a new PR for each component? P.S. I made a small fix to |
It would be easier to do it in one PR. @tobiasschweizer ? |
|
After talking with @tobiasschweizer about the complexity of all of this, we have decided to split it into one PR per value component to make the reviewing process easier. |
@mdelez I'll do the review one Monday |
For the reviewers, to test:
|
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.
See my comments on testing.
@@ -116,6 +117,26 @@ class TestHostCreateValueComponent implements OnInit { | |||
} | |||
} | |||
|
|||
/** |
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.
Could you also add a test to projects/dsp-ui/src/lib/viewer/values/date-value/date-input/date-input.component.spec.ts
? Maybe just check for the form to be valid.
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.
done in c05e0fd
}); | ||
|
||
it('should not create an empty value', () => { | ||
expect(testHostComponent.inputValueComponent.getNewValue()).toEqual(false); |
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.
You could also a test to check that valueRequiredValidator
is correctly propagated to TestDateInputComponent
(binding in projects/dsp-ui/src/lib/viewer/values/date-value/date-value.component.html
).
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.
done in c05e0fd
expect(testHostComponent.inputValueComponent).toBeTruthy(); | ||
}); | ||
|
||
it('should not create an empty value', () => { |
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.
Maybe also check that the form is valid.
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.
done in c05e0fd
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 have a question: when you check the box "is a time period", we get an error message to indicate how to fill in the fields. I guess I will get the same problem in my PR if a user checks this box... Is there a possibility to turn it into an hint instead of an error??
I fixed the logic in d113e0b. Thanks for pointing it out :) I kept them as error messages but now they only appear when they should. |
done in 2f243dd |
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.
Looks good!
resolves DSP-1188