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(datetime): recalculate day column when month is changed #17815

Merged
merged 14 commits into from
Mar 21, 2019

Conversation

liamdebeasi
Copy link
Contributor

@liamdebeasi liamdebeasi commented Mar 18, 2019

Short description of what this resolves:

This PR is meant to combine the changes proposed by #17562 and #14434 and to clean up the code a bit. Additionally, this PR also introduces more tests

Changes proposed in this pull request:

  • Add onIonPickerColChange event to ion-picker-column to allow the datetime component to be aware of the need to recalculate day values
  • Add tests for ion-picker-column
  • Add generic test utilities for interaction and listening for evens.

Ionic Version:

Fixes:
#14233
#14732
#15452
#15794
#16733
#17060
#17510
#17521

@ionitron-bot ionitron-bot bot added the package: core @ionic/core package label Mar 18, 2019
@liamdebeasi liamdebeasi marked this pull request as ready for review March 18, 2019 19:55
@liamdebeasi liamdebeasi changed the title Datetime col change fix(datetime): recalculate day column when month is changed Mar 19, 2019
@brandyscarney
Copy link
Member

brandyscarney commented Mar 19, 2019

When we go to merge this, can we add @KillerCodeMonkey and @olivercodes as co-authors if this is going to close their PRs? https://help.github.com/en/articles/creating-a-commit-with-multiple-authors

@KillerCodeMonkey
Copy link
Contributor

@liamdebeasi @brandyscarney nice this topic is on its way 🚀

If i can support you here, let me know :)

@brandyscarney
Copy link
Member

@KillerCodeMonkey Thank you! 🙂 So we found a bug with this PR but we're scheduled to release today so we aren't going to get this in today's release, but we're going to do our normal release and then release a dev build with this PR in it so we can get some community testing. We'll post the dev release here and on some of the datetime issues once it's out.

@KillerCodeMonkey
Copy link
Contributor

@brandyscarney

ah okay. so if you need help to solve the bug or need someone to test it 😉 .
I am happy you have this on your agenda, now.

@@ -325,6 +325,7 @@ export class Datetime implements ComponentInterface {
text: this.cancelText,
role: 'cancel',
handler: () => {
this.updateDatetimeValue(this.value);
Copy link
Contributor

Choose a reason for hiding this comment

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

@liamdebeasi thats why i stored the changed value on an internal State in my PR (only as an info) :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense! I'd just rather reset it than store two copies of data that could potentially differ

@KillerCodeMonkey
Copy link
Contributor

KillerCodeMonkey commented Mar 20, 2019 via email

@liamdebeasi liamdebeasi merged commit 9273f97 into master Mar 21, 2019
@KillerCodeMonkey
Copy link
Contributor

KillerCodeMonkey commented Mar 21, 2019 via email

liamdebeasi added a commit that referenced this pull request Mar 21, 2019
…17815)

Co-Authored-By: KillerCodeMonkey <bengtler@gmail.com>
Co-Authored-By: olivercodes <boliver@linux.com>
Co-Authored-By: liamdebeasi <liamdebeasi@users.noreply.github.com>
liamdebeasi added a commit that referenced this pull request Mar 21, 2019
@liamdebeasi
Copy link
Contributor Author

I was missing a space when adding your name/email for co-authors so I'm going to revert and try it again 🙂

@KillerCodeMonkey
Copy link
Contributor

KillerCodeMonkey commented Mar 21, 2019 via email

liamdebeasi added a commit that referenced this pull request Mar 21, 2019
@liamdebeasi liamdebeasi deleted the datetime-col-change branch March 21, 2019 13:08
@zxdeer
Copy link

zxdeer commented Apr 19, 2019

Short description of what this resolves:

This PR is meant to combine the changes proposed by #17562 and #14434 and to clean up the code a bit. Additionally, this PR also introduces more tests

Changes proposed in this pull request:

  • Add onIonPickerColChange event to ion-picker-column to allow the datetime component to be aware of the need to recalculate day values
  • Add tests for ion-picker-column
  • Add generic test utilities for interaction and listening for evens.

Ionic Version:

Fixes:
#14233
#14732
#15452
#15794
#16733
#17060
#17510
#17521

How use in Ionic?

@liamdebeasi
Copy link
Contributor Author

Hi there,

Please visit the Ionic DateTime Documention for instructions on how to use this component.

Thanks!

@olivercodes
Copy link
Contributor

Thank you for the co-author marks @liamdebeasi @brandyscarney, much appreciated

kiku-jw pushed a commit to kiku-jw/ionic that referenced this pull request May 16, 2019
…onic-team#17815)

Co-Authored-By: KillerCodeMonkey<bengtler@gmail.com>
Co-Authored-By: olivercodes <boliver@linux.com>
Co-Authored-By: liamdebeasi <liamdebeasi@users.noreply.github.com>
kiku-jw pushed a commit to kiku-jw/ionic that referenced this pull request May 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants