-
Notifications
You must be signed in to change notification settings - Fork 616
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
Formik upgrade #3469
Formik upgrade #3469
Conversation
be11a8b
to
67984a8
Compare
/retest |
67984a8
to
e123775
Compare
cc @rohitkrai03 |
e123775
to
e8cfb62
Compare
e8cfb62
to
e386694
Compare
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.
Upgrade looks good. Please to see the refactor PR make it in that depends on this PR.
/lgtm
@jtomasek Any specific reason for upgrading Formik? I had kept the formik version to rc1 because of an issue with low priority validations being done on stale values which would result in error message of that input field to appear late. Few related issues on formik repository -
Weirdly enough the issue has not been fixed even after 13 rcs and a few more releases of formik after that. Just tested the PR for it and you can see the effects of this bug in below screenshot. When the scaling input goes to We would need to spend some time finding a solution that works for us for this specific issue after the upgrade. |
It looks like there is some workarounds in those issues (such as invoking the validation ourselves - |
My goal was simply to move from RCs and upgrade Formik to the new 'stable' 2.0.3 version and add new multi checkbox functionality. All this before moving the components tho |
@jtomasek Not that I am currently asking you to do so... but I am wondering, could you do the migration to |
No major conflicts it seems. I can rebase that PR so it does not depend on this upgrade. |
I leave it up to you - I think the move to |
Done (#3516). |
@andrewballantyne I did try using |
Since this is separated now, perhaps we address this issue before upgrading. /lgtm cancel |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jtomasek The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@jtomasek is this still needed? Is it worth salvaging anything here in the current PR or closing and creating a new PR? |
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
@jtomasek: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
/close Looks like this is not going to go in, closing PR. |
@andrewballantyne: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Upgrade Formik to version 2.0.3
Required by #3516