-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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(datetime): add dev warnings when setting out of bounds value #25513
Conversation
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.
Tested locally and it does exactly what it needs to do. Great work 👍 I agree that adding tests for the contents of the warning message has been flaky and does not introduce value here.
~ Would there be any value to the developer if the warning message included the values back to them? For example:
[Ionic Warning]: The value (2021-05-01) passed to ion-datetime is earlier than the min (2021-06-01) or later than the max (2021-10).
or:
[Ionic Warning]: The value passed to ion-datetime is earlier than the min or later than the max. { min: "2021-06-01", value: "2021-05-01", max: "2021-06-30" }
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.
Good to go once the comment below is addressed
If it's not too hard, I think it would provide for a nice DX to have a more specific message. Could do something like this:
At least for me, listing the values on separate lines makes the warning easy to scan. |
Agreed, I was thinking just use the second argument of |
I like that multiline formatting 👀 I'll pull it into a separate util too. |
I went with logging the min/max/value as stringified objects, so we don't have to worry about date formatting across locales. |
Pull request checklist
Please check if your PR fulfills the following requirements:
ionic-docs
repo, in a separate PR. See the contributing guide for details.npm run build
) was run locally and any changes were pushednpm run lint
) has passed locally and any fixes were made for failuresPull request type
Please check the type of change your PR introduces:
What is the current behavior?
Originally, assigning a
value
toion-datetime
that's sufficiently earlier than themin
or later than themax
would lead to a disabled month being displayed, and the user being unable to navigate in either direction. This was mostly resolved here #25351 since both assigning a value and leaving it to the default go through theprocessValue
function. The dev experience just needs some polish.Issue URL: #24881
What is the new behavior?
Console warnings for out-of-bounds values are now printed when either assigning an initial
value
, or updating it programmatically.Does this introduce a breaking change?
Other information
Tests for console warnings have been flaky in the past, so I didn't include one. I did add a test to ensure an in-bounds month is displayed, though, since the existing tests only cover datetimes with no initial value.