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

[Core] Date field js validation #7266

Merged
merged 1 commit into from
Aug 26, 2021

Conversation

laemtl
Copy link
Contributor

@laemtl laemtl commented Dec 24, 2020

For one of my projects, I need to validate a date field in React and display a front-end error message. Select fields have an errorMessage and hasError prop for such scenarios, but those are not implemented on DateElement. This PR implements the same logic on DateElement.

@laemtl laemtl added 23.0.0-testing Proposal PR or issue suggesting an improvement that can be accepted, rejected or altered Area: UI PR or issue related to the user interface and removed 23.0.0-testing Proposal PR or issue suggesting an improvement that can be accepted, rejected or altered labels Dec 24, 2020
@driusan
Copy link
Collaborator

driusan commented Jan 4, 2021

Why is this going to 23?

@laemtl
Copy link
Contributor Author

laemtl commented Jan 4, 2021

I'm using v23 in the project I implemented this for.

onUserInput: PropTypes.func,
};

DateElement.defaultProps = {
name: '',
label: '',
value: '',
value: undefined,
Copy link
Member

@maltheism maltheism Jan 4, 2021

Choose a reason for hiding this comment

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

@laemtl should value be undefined? Line 1054 still checks this.props.value === '' for if empty string. I'm wondering if null is more appropriate instead of undefined if empty string isn't preferred for some reason?

Copy link
Contributor Author

@laemtl laemtl Jan 4, 2021

Choose a reason for hiding this comment

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

If the value is set to '', an error is detected on page load (the initial value enters the error test case on line 1054). Value can be null instead, but other elements (select for example) use undefined as their initial value. Any reason why undefined should be avoided?

Copy link
Member

@maltheism maltheism Jan 4, 2021

Choose a reason for hiding this comment

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

I've not witnessed undefined from an input tag before and so I was just curious. I get an empty string when the variable has not been set for <input> if I make one and try to console.log the value. I'm guessing date works differently.

@kongtiaowang kongtiaowang added the Passed manual tests PR has been successfully tested by at least one peer label Jan 8, 2021
@ridz1208 ridz1208 changed the base branch from 23.0-release to main February 11, 2021 13:48
@ridz1208
Copy link
Collaborator

@laemtl I have changed the base on github from 23 to main so this PR stops showing up when we query the 23 branch. The PR now shows a bunch of files changed/commits/conflicts. dont worry about these for now, as soon as we puch 23 into main (after the release) they should auto resolve and if not a minor rebase should do the trick.

@laemtl laemtl added the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Feb 12, 2021
@driusan
Copy link
Collaborator

driusan commented Apr 22, 2021

@laemtl can you rebase this? It's both showing the wrong changes since it was originally sent to a different branch and needs to have the tests run on php8.

@cmadjar
Copy link
Collaborator

cmadjar commented Jun 8, 2021

@laemtl could you rebase the PR please? Thank you!

@laemtl laemtl force-pushed the 2020-12-23-date-field-js-validation branch from 87f2676 to 466e103 Compare June 8, 2021 20:51
@laemtl
Copy link
Contributor Author

laemtl commented Jun 8, 2021

@cmadjar Rebased.

@laemtl laemtl removed the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Jun 8, 2021
@driusan
Copy link
Collaborator

driusan commented Jun 15, 2021

@laemtl the diff looks better now but it looks like the tests are failing since rebasing

@laemtl laemtl force-pushed the 2020-12-23-date-field-js-validation branch from 466e103 to 3d34ec5 Compare June 21, 2021 15:50
@cmadjar
Copy link
Collaborator

cmadjar commented Aug 10, 2021

@driusan ready for you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: UI PR or issue related to the user interface Passed manual tests PR has been successfully tested by at least one peer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants