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: fields should not remove initially set invalid status on ready #418

Merged
merged 3 commits into from Oct 2, 2019

Conversation

Haprog
Copy link
Contributor

@Haprog Haprog commented Sep 27, 2019

Fixes #417.

Haprog added a commit to vaadin/vaadin-custom-field that referenced this pull request Sep 27, 2019
Update "alignment" visual test screenshots to match proper behaviour
once this has been merged and released:
vaadin/vaadin-text-field#418

Update out of date visual test screenshot: "form-layout" (material,
Firefox) which doesn't seem to have any noticeable change, but was
failing.
@web-padawan web-padawan self-requested a review October 1, 2019 07:01
Copy link
Member

@web-padawan web-padawan left a comment

Choose a reason for hiding this comment

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

I have some concerns related to the suggested change:

  1. If we are sure that _constraintsChanged observer needs to be called after ready, it makes sense to initialise it on ready() rather than in constructor(). Then we don't need a flag.

  2. I agree with the statement from fix: stuck in invalid state after last validation constraint is removed while invalid #401 (comment) regarding the observer:

(could be extended by numberfield if -mixin shouldn't know about min, max, step):

Please see c21b64b for solution taking these into account (split-constrains-observer branch).

@Haprog
Copy link
Contributor Author

Haprog commented Oct 2, 2019

@web-padawan Great solution, thanks! I was initially trying to do something similar but I couldn't figure out a way to override the observer defined in a superclass. I didn't think of creating it in another (overridable) method and calling it in ready().

I fast-forwarded this PR branch to contain your changes as is.

@Haprog Haprog requested review from web-padawan and removed request for tomivirkki October 2, 2019 06:00
@web-padawan web-padawan merged commit a3d2665 into master Oct 2, 2019
@web-padawan web-padawan deleted the fix/417 branch October 2, 2019 06:29
@web-padawan
Copy link
Member

web-padawan commented Oct 2, 2019

This needs to be cherry-picked to 2.4 branch and released (as master is already 2.5 alpha)

@Haprog
Copy link
Contributor Author

Haprog commented Oct 2, 2019

Cherry-picked to 2.4 and released as v2.4.12

web-padawan pushed a commit to vaadin/vaadin-custom-field that referenced this pull request Oct 2, 2019
Update "alignment" visual test screenshots to match proper behaviour
once this has been merged and released:
vaadin/vaadin-text-field#418

Update out of date visual test screenshot: "form-layout" (material,
Firefox) which doesn't seem to have any noticeable change, but was
failing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fields should not remove initially set invalid status on ready
2 participants