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(textfield): reportValidity() doesn't work #760

Merged
merged 2 commits into from
Apr 12, 2021

Conversation

tveinfeld
Copy link
Contributor

…ttribute values before altering them (resolved viv-481)
@github-actions
Copy link

🚀

Latest successful build of the PR deployed here.

🚀

@tveinfeld tveinfeld changed the title fix(textfield): reportValidation doesn't work fix(textfield): reportValidity() doesn't work Apr 11, 2021
Copy link
Contributor

@yinonov yinonov left a comment

Choose a reason for hiding this comment

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

I want to deep dive on this, just for "education". would u please add relevant tests? it is quite a fragile component now

@gullerya
Copy link
Contributor

gullerya commented Apr 12, 2021

I suggest to slightly simplify the implementation (and also fix some more case which is not yet fixed, case when value = 0 or empty string):

	if (value === undefined) {
		target.removeAttribute(attributeName);
	} else {
		const normalizedValue = asEmpty ? '' : String(value);
		if (target.getAttribute(attributeName) !== normalizedValue) {
			target.setAttribute(attributeName, normalizedValue);
		}
	}

@tveinfeld
Copy link
Contributor Author

I suggest to slightly simplify the implementation (and also fix some more case which is not yet fixed, case when value = 0 or empty string):

	if (value === undefined) {
		target.removeAttribute(attributeName);
	} else {
		const normalizedValue = asEmpty ? '' : String(value);
		if (target.getAttribute(attributeName) !== normalizedValue) {
			target.setAttribute(attributeName, normalizedValue);
		}
	}

As discussed "online", I'd rather keep the original function's interface/contract with user as is for now, changing it would be out of this PR's scope.

@sonarcloud
Copy link

sonarcloud bot commented Apr 12, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@tveinfeld
Copy link
Contributor Author

@yinon: I have used @gullerya's regression tests, feel free to review again 🙂

@gullerya
Copy link
Contributor

The suggestion about the above implementation is still effective from my POV :)
To not change the signature, can remove the === undefined part, but still the implementation is much simpler and readable, IMHO.

@tveinfeld
Copy link
Contributor Author

The suggestion about the above implementation is still effective from my POV :)
To not change the signature, can remove the === undefined part, but still the implementation is much simpler and readable, IMHO.

I would argue about the simplicity and readability of your suggested code, plus I don't want to introduce any new logic, so I'd rather not alter the PR. LMK if it's an absolute "no-go" for you - or open a separate PR for the added new logic suggested.

@gullerya gullerya merged commit 600f26a into master Apr 12, 2021
@gullerya gullerya deleted the viv-481-report-validity branch April 12, 2021 08:33
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.

3 participants