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(ion-datetime): convert DatetimeData to ISO string #15833

Closed
wants to merge 1 commit into from

Conversation

kensodemann
Copy link
Member

Short description of what this resolves:

Modifies the datetime component to emit the ISO string for the value changed when the value changes. The value itself gets set to the picker structure, so that is no good as something to emit.

NOTE: it may at some point be better to rewrite the component to not use this.value as the value returned from the picker, but that would be a much larger and much more error prone rewrite at this point. I would want proper tests in place first.

Changes proposed in this pull request:

  • emit the ISO date representation of the value rather than the value itself
  • modify the CVA to pick up the emitted value rather than the actual value in the target
  • start adding unit tests

Ionic Version: 4.x

Fixes: #15408

@ionitron-bot ionitron-bot bot added package: angular @ionic/angular package package: core @ionic/core package labels Oct 2, 2018
Copy link
Contributor

@manucorporat manucorporat left a comment

Choose a reason for hiding this comment

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

The PR looks great, but wondering if event.detail.value !== event.target.value could be a problem. The rest of input component maintain a consistency regarding value

@kensodemann
Copy link
Member Author

I believe what I did in PR #15907 to fix this makes a lot more sense, so I am going to close this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: angular @ionic/angular package package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants