-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
Add validation to new settings editor #56176
Add validation to new settings editor #56176
Conversation
Looks like there's some tests that need updating |
…tings-validation
Never mind, looks like that's unrelated, despite being a tree issue. |
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.
There are some merge conflicts, some things in settingsTree.ts moved to settingsTreeModels.ts, I can help you fix it up if needed.
} else if (isArray(setting.type) && setting.type.indexOf('null') > -1 && setting.type.length === 2) { | ||
if (setting.type.indexOf('number') > -1) { | ||
element.valueType = 'nullable-integer'; | ||
} else if (setting.type.indexOf('number') > -1) { |
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.
number vs integer?
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.
And what about types that fall out of this check and end up with no valueType?
@@ -909,8 +909,7 @@ class SettingsContentBuilder { | |||
|
|||
setting.descriptionRanges = []; | |||
const descriptionPreValue = indent + '// '; | |||
for (let line of setting.description) { | |||
// Remove setting link tag | |||
for (let line of (setting.deprecationMessage ? [setting.deprecationMessage] : setting.description)) { |
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.
It only writes the deprecation message instead of the description?
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.
Could make it write both, but the concern from earlier was that it was not totally clear that the item was deprecated. Maybe it would be better with the deprecation message at the top? What do you think?
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.
I think it should have both. How about just prepending the deprecationMessage in *
so it gets rendered in italics? And we can decide whether we want something fancier later.
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.
As discussed, this is the raw text model so we dont have italic support. In the new editor we use the special deprecation warning element anyways
|
…tings-validation
…tings-validation
@roblourens I think this should be ready to go. |
Closes #55801