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

Address feedback in #101467 #105880

Merged
merged 2 commits into from
Sep 2, 2020
Merged

Conversation

IllusionMH
Copy link
Contributor

This PR addresses feedback in #101467 (comment)

Couple of notes:

  1. enum now only used to provide autocompletion suggestions and renamed accordingly.
  2. Apparently errorMessage will be shown only from first item in anyOf
  3. type: 'number' should be first otherwise number validation Value is above the maximum of 1000. won't be shown for numbers, however it can be moved lower to have same error message for 1001 and "10001"

/cc @alexdima @jrieken

@alexdima
Copy link
Member

alexdima commented Sep 2, 2020

Thank you!

@alexdima alexdima added this to the August 2020 milestone Sep 2, 2020
@alexdima alexdima merged commit 95e7497 into microsoft:master Sep 2, 2020
@IllusionMH IllusionMH deleted the feedback-for-101467 branch September 2, 2020 21:44
@IllusionMH IllusionMH restored the feedback-for-101467 branch September 2, 2020 21:59
@IllusionMH
Copy link
Contributor Author

@alexdima I've remembered why I haven't used clampedInt - font-weight: 10000 will be totally ignored by browser therefore my initial check was min <= val <= max to match browser behavior. Demo

I think it's better to match browser behavior and revert code back to min <= val <= max probably add explicit isNaN check if needed.

@alexdima
Copy link
Member

alexdima commented Sep 2, 2020

I agree that this is the behaviour of CSS, but these are editor settings, the behaviour of out-of-range values doesn't need to conform to the CSS specification. I think it is OK if we are consistent with our other settings, so we clamp small/large values. IIRC we do the same for fontSize and other editor settings.

@IllusionMH IllusionMH deleted the feedback-for-101467 branch September 2, 2020 22:34
@github-actions github-actions bot locked and limited conversation to collaborators Oct 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants