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

Templates editing updates #168

Merged
merged 6 commits into from
Jan 9, 2024
Merged

Templates editing updates #168

merged 6 commits into from
Jan 9, 2024

Conversation

smithgp
Copy link
Contributor

@smithgp smithgp commented Jan 8, 2024

What does this PR do?

  • Add datasetFileTemplate to rules appliesTo
  • Add tile visibility to json schema (available in the Spring '24 release)
  • Warn on empty validate page group
  • Add Component page layout type, including completion, definition, and hover support for variables (available in the Spring '24 release)
  • Remove non-arrow-function return for tslint
  • Fix json path matching to be exact matching

What issues does this PR fix or reference?

@W-14498704@, @W-14496669@, @W-14516604@, @W-14306583@

This is available in the Spring '24 release.
This is available in the Spring '24 release.
to make our old tslint version happy, plus we don't need to support propagating
'this' here (and someone could just use a bound method if they did).
The builtin matches() method from jsonc actually does the equivalent of a
starts-with, while in most cases we want to match on an exact json path (e.g.
right to a field, and not to a subfield of the final part of the path pattern).
@smithgp smithgp requested a review from a team January 8, 2024 03:06
@smithgp smithgp self-assigned this Jan 8, 2024
// values under "values")
location.path.length === 4
location.isAtPropertyKey && locationMatches(location, ['configuration', 'appConfiguration', 'values', '*'])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the change in jsoncUtils.ts down below for what all these locationMatches() changes are about.

@@ -382,38 +396,24 @@ describe('TemplateEditorManager configures layoutDefinition', () => {
await waitForDiagnostics(uri, d => d && d.length >= 4);
await waitForTemplateEditorManagerHas(await getTemplateEditorManager(), uriDirname(uri), true);

const position = findPositionByJsonPath(doc, ['pages', 0, 'layout', 'center', 'items', 0, 'name']);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To avoid duplicating code too much, I moved this logic into a function.

@@ -455,126 +455,69 @@ describe('TemplateEditorManager configures layoutDefinition', () => {
await waitForDiagnostics(uri, d => d && d.length >= 4);
await waitForTemplateEditorManagerHas(await getTemplateEditorManager(), uriDirname(uri), true);

const position = findPositionByJsonPath(doc, ['pages', 0, 'layout', 'center', 'items', 0, 'name']);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here, made function to avoid code duplication.

@@ -534,7 +547,7 @@
]
},
"visibility": {
"description": "Controls if this item is visible. Value must be 'Disabled' (valid only for Variable items), 'Hidden', 'Visible', or a {{...}} expression against variables that evaluates to one of those or true (Visible) or false (Hidden).",
"description": "Controls if this item is visible. Value must be 'Disabled' (valid only for Variable items and tiles), 'Hidden', 'Visible', or a {{...}} expression against variables that evaluates to one of those or true (Visible) or false (Hidden).",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mblumreich - These descriptions in this schema file will show as hover text in vscode.

let result: R;
let resultError: unknown | undefined;
let _fn: ((this: T, ...args: A) => R) | undefined = fn;
return function (this: T, ...args: A) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

tslint was spitting out an error (but not failing) about on this return of a function rather than a lambda. We don't need the this handling here anyways, so I just switched it to a lambda.

Copy link
Contributor

@aarondailsf2023 aarondailsf2023 left a comment

Choose a reason for hiding this comment

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

Looks good to me

@@ -76,7 +77,7 @@ export class JsonAttributeRelFilePathDefinitionProvider extends JsonAttributeDef
location.previousNode &&
location.previousNode.type === 'string' &&
location.previousNode.value &&
this.patterns.some(location.matches)
this.patterns.some(p => locationMatches(location, p))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should you pass in false for the exact parameter, so it matches the existing behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, I want the new exact match behavior here (like in almost all the places), since it's the correct thing.

@smithgp smithgp merged commit afdfe9f into develop Jan 9, 2024
7 checks passed
@smithgp smithgp deleted the iadp/template-schema-updates branch January 9, 2024 22:49
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.

2 participants