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

Add theme block type completion in {% schema %} #632

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

charlespwd
Copy link
Contributor

@charlespwd charlespwd commented Dec 2, 2024

What are you adding in this PR?

The BlockTypeCompletionProvider offers value completions of the blocks.[].type property inside section and theme block {% schema %} tags.

{% schema %}
{
  "blocks": [
    { "type": "" },
  ]
}
{% endschema %}

Fixes #627

block-type-completion.mp4

What's next? Any followup issues?

#545

Before you deploy

  • I included a minor bump changeset

The `BlockTypeCompletionProvider` offers value completions of the
`blocks.[].type` property inside section and theme block `{% schema %}` tags.

```liquid
{% schema %}
{
  "blocks": [
    { "type": "█" },
  ]
}
{% endschema %}
```

Fixes #627
@charlespwd charlespwd force-pushed the feature/block-type-completion-example branch from 13ff1da to b041721 Compare December 2, 2024 16:20
Comment on lines +69 to +71
export function isPresetBlockPath(path: JSONPath) {
return path.at(0) === 'presets';
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unused but might want to refactor and use for #628

Comment on lines +76 to +82
// - presets.[](recursive .blocks.[].type)
// - presets.[](recursive .blocks.{}.type)
const topLevel = path.at(0);
if (topLevel !== 'blocks' && topLevel !== 'presets') return false;
if (path.length < 4) return false; // minimum path length
const [shouldBeBlocks, _idOrIndex, shouldBeType] = path.slice(-3);
return shouldBeBlocks === 'blocks' && shouldBeType === 'type';
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 that was something I used before I planned to split #628 to a different provider

Copy link
Contributor

@graygilmore graygilmore left a comment

Choose a reason for hiding this comment

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

🎩 went well, just a few small things

@@ -166,7 +166,11 @@ export class DocumentManager {

/** Lazy and only computed once per file version */
getSchema: memo(async () => {
if (!this.getModeForUri || !this.isValidSchema) return undefined;
// For when you don't care about validation... (e.g. tests)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could only return this is a test env?

Comment on lines +59 to +60
label: `"${name}"`,
insertText: `"${name}"`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
label: `"${name}"`,
insertText: `"${name}"`,
label: name,
insertText: name,

My eyes might be deceiving me but I think we can just do this?

} from 'vscode-languageserver-protocol';
import { DocumentManager } from '../../documents';

export function getParams(
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these really "params"? Maybe I need to rethink what I think params are in theme-tools context?

Edit: ah, I see these were just copy/pasted from an existing file anyways

expect(completions.items.map((item) => item.label)).to.include.members([
'"@app"',
'"@theme"',
...blockNames.map((x) => `"${x}"`),
Copy link
Contributor

Choose a reason for hiding this comment

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

Interpolation overload perhaps?

Suggested change
...blockNames.map((x) => `"${x}"`),
...blockNames,

Comment on lines +124 to +126
expect(completions.items.map((item) => item.label)).not.to.include.members([
...blockNames.map((x) => `"${x}"`),
]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expect(completions.items.map((item) => item.label)).not.to.include.members([
...blockNames.map((x) => `"${x}"`),
]);
expect(completions.items.map((item) => item.label)).not.to.include.members(blockNames);

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.

Theme block type (definition) completion
2 participants