Skip to content
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

Merged
merged 5 commits into from Jan 11, 2021

Conversation

mdelez
Copy link
Contributor

@mdelez mdelez commented Jan 7, 2021

resolves DSP-1188

@mdelez mdelez added bug Something isn't working refactor labels Jan 7, 2021
@mdelez mdelez self-assigned this Jan 7, 2021
@mdelez
Copy link
Contributor Author

mdelez commented Jan 7, 2021

@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 int-value-component.spec in this commit as well 🙊

@flavens
Copy link
Collaborator

flavens commented Jan 8, 2021

@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?

It would be easier to do it in one PR. @tobiasschweizer ?

@tobiasschweizer
Copy link
Contributor

tobiasschweizer commented Jan 8, 2021

One PR seems less complicated.
Two PRs will facilitate reviewing

@mdelez
Copy link
Contributor Author

mdelez commented Jan 8, 2021

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.

@tobiasschweizer
Copy link
Contributor

@mdelez I'll do the review one Monday

@mdelez
Copy link
Contributor Author

mdelez commented Jan 8, 2021

For the reviewers, to test:

  • add [valueRequiredValidator]="false" to dsp-date-value in add-value.component.html
  • check that when you click on the add button in the playground for a new date value, you shouldn't see any error messages initially and the save icon should be enabled (although it will fail on knora's end if you click it)
  • change [valueRequiredValidator]="false" to true
  • check that the error messages behave correctly and that the save icon is disabled

@mdelez mdelez removed the refactor label Jan 8, 2021
Copy link
Contributor

@tobiasschweizer tobiasschweizer left a 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 {
}
}

/**
Copy link
Contributor

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.

Copy link
Contributor Author

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);
Copy link
Contributor

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).

Copy link
Contributor Author

@mdelez mdelez Jan 11, 2021

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', () => {
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in c05e0fd

Copy link
Collaborator

@flavens flavens left a 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??

@mdelez mdelez changed the title DSP-1188 - complex value validators fix(Date Value Validators): Propagate valueRequiredvalidator to child component (DSP-1188) Jan 11, 2021
@mdelez mdelez changed the title fix(Date Value Validators): Propagate valueRequiredvalidator to child component (DSP-1188) fix(Date Value Validators): Propagate valueRequiredValidator to child component (DSP-1188) Jan 11, 2021
@mdelez
Copy link
Contributor Author

mdelez commented Jan 11, 2021

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.

@mdelez
Copy link
Contributor Author

mdelez commented Jan 11, 2021

maybe you could add the same test case as above in "'should mark the form's validity correctly'", however this time it is valid.

done in 2f243dd

Copy link
Contributor

@tobiasschweizer tobiasschweizer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@mdelez mdelez merged commit 27502f5 into main Jan 11, 2021
@mdelez mdelez deleted the wip/DSP-1188-complex-value-validators branch January 11, 2021 13:31
@mpro7 mpro7 mentioned this pull request Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants