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

Space around class_list #418

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion docs/contributing.md
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,9 @@ If ever you want to see how the VS Code extension or playground would behave bef
theme-docs download
```

6. Proceed to debug the VS Code extension or playground as usual.
6. Note that you may have to delete the cached data directory (`~/Library/Caches/theme-liquid-docs-nodejs`) in order to see your changes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is general contribution docs, we should probably be a bit more specific about what this instruction is alluding to.

Suggested change
6. Note that you may have to delete the cached data directory (`~/Library/Caches/theme-liquid-docs-nodejs`) in order to see your changes.
6. Note when working on changes to theme-liquid-docs: You will have to delete the cached data directory (`~/Library/Caches/theme-liquid-docs-nodejs`) in order to see changes made to `packages/theme-check-docs-updater/data`.


7. Proceed to debug the VS Code extension or playground as usual.

## Submitting a Pull Request

Expand Down
2 changes: 1 addition & 1 deletion packages/theme-check-common/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@shopify/theme-check-common",
"version": "2.8.0",
"version": "2.9.0",
geddski marked this conversation as resolved.
Show resolved Hide resolved
"license": "MIT",
"main": "dist/index.js",
"types": "dist/index.d.ts",
Expand Down
2 changes: 2 additions & 0 deletions packages/theme-check-common/src/checks/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import { PaginationSize } from './pagination-size';
import { ParserBlockingScript } from './parser-blocking-script';
import { RemoteAsset } from './remote-asset';
import { RequiredLayoutThemeObject } from './required-layout-theme-object';
import { SpaceAfterClassList } from './space-after-class_list';
import { TranslationKeyExists } from './translation-key-exists';
import { UnclosedHTMLElement } from './unclosed-html-element';
import { UndefinedObject } from './undefined-object';
Expand Down Expand Up @@ -59,6 +60,7 @@ export const allChecks: (LiquidCheckDefinition | JSONCheckDefinition)[] = [
ParserBlockingScript,
RemoteAsset,
RequiredLayoutThemeObject,
SpaceAfterClassList,
TranslationKeyExists,
UnclosedHTMLElement,
UndefinedObject,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
import { expect, describe, it } from 'vitest';
import { SpaceAfterClassList } from '.';
import { runLiquidCheck, highlightedOffenses } from '../../test';

describe('Module: SpaceAfterClassList', () => {
it('allows the happy path', async () => {
const file = `<div class="{{ block.settings | class_list }}">
content
</div>
`;

const offenses = await runLiquidCheck(SpaceAfterClassList, file);

expect(offenses).to.have.length(0);
});

it('should report an offense when missing a space', async () => {
const file = `<div class="{{ block.settings | class_list }}other-class">
content
</div>
`;

const offenses = await runLiquidCheck(SpaceAfterClassList, file);

expect(offenses).to.have.length(1);
expect(offenses[0].message).toEqual(
`Missing a space after using the class_list filter: 'block.settings | class_list }}other-class'`,
);

const highlights = highlightedOffenses({ 'file.liquid': file }, offenses);
expect(highlights).toEqual(['o']);
});

it('supports classes on new lines', async () => {
const file = `<div class="
{{ block.settings | class_list }}other-class">
content
</div>
`;

const offenses = await runLiquidCheck(SpaceAfterClassList, file);

expect(offenses).to.have.length(1);
expect(offenses[0].message).toEqual(
`Missing a space after using the class_list filter: 'block.settings | class_list }}other-class'`,
);

const highlights = highlightedOffenses({ 'file.liquid': file }, offenses);
expect(highlights).toEqual(['o']);
});

it('supports arbitrary whitespace', async () => {
const file = `<div class="{{block.settings| class_list}}other-class">
content
</div>
`;

const offenses = await runLiquidCheck(SpaceAfterClassList, file);

expect(offenses).to.have.length(1);
expect(offenses[0].message).toEqual(
`Missing a space after using the class_list filter: 'block.settings| class_list}}other-class'`,
);

const highlights = highlightedOffenses({ 'file.liquid': file }, offenses);
expect(highlights).toEqual(['o']);
});

it('catches between multiple class_list filters', async () => {
const file = `<div class="
{{ block.settings.spacing | class_list }}oh-no
{{ block.settings.typography | class_list }}">
content
</div>
`;

const offenses = await runLiquidCheck(SpaceAfterClassList, file);
expect(offenses).to.have.length(1);

expect(offenses[0].message).toEqual(
`Missing a space after using the class_list filter: 'block.settings.spacing | class_list }}oh-no'`,
);

const highlights = highlightedOffenses({ 'file.liquid': file }, offenses);
expect(highlights).toEqual(['o']);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
import { NodeTypes } from '@shopify/liquid-html-parser';
import { LiquidCheckDefinition, Severity, SourceCodeType } from '../../types';

export const SpaceAfterClassList: LiquidCheckDefinition = {
meta: {
code: 'SpaceAfterClassList',
aliases: ['SpaceAfterClassList'],
name: 'Space After Class List',
docs: {
description: 'Warns you when there is no space after using the class_list filter',
recommended: true,
url: 'https://shopify.dev/docs/themes/tools/theme-check/checks/space-after-class_list',
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there another PR I can check out for this?

Copy link
Author

Choose a reason for hiding this comment

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

knew I was forgetting something! I'll see how to add that one sec

Copy link
Author

Choose a reason for hiding this comment

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

},
type: SourceCodeType.LiquidHtml,
severity: Severity.ERROR,
geddski marked this conversation as resolved.
Show resolved Hide resolved
schema: {},
targets: [],
},

create(context) {
return {
async LiquidFilter(node, ancestors) {
if (node.name !== 'class_list') {
return;
}

const classAttribute = ancestors.find(
(ancestor) =>
ancestor.type === NodeTypes.AttrDoubleQuoted ||
ancestor.type === NodeTypes.AttrSingleQuoted,
);

if (!classAttribute) {
return;
}

const classAttributeContent =
classAttribute.source.slice(classAttribute.position.start, classAttribute.position.end) ||
'';

const regex = /([a-zA-Z0-9._-]+)\s*\|\s*class_list\s*}}([a-zA-Z0-9._-]+)/gm;

const matches = [...classAttributeContent.matchAll(regex)];

for (const match of matches) {
if (match.index === undefined) {
continue;
}

const liquidVariable = ancestors.find(
(ancestor) => ancestor.type === NodeTypes.LiquidVariable,
);
const liquidVariableContent =
liquidVariable?.source.slice(
liquidVariable.position.start,
liquidVariable.position.end,
) || '';
const styleSetting = liquidVariableContent.split('|')[0]?.trim();

if (styleSetting !== match[1]) {
continue;
}

const bracketIndex = match[0].indexOf('}}');
const errorPosition = classAttribute.position.start + match.index + bracketIndex + 2;

context.report({
message: `Missing a space after using the class_list filter: '${match[0]}'`,
startIndex: errorPosition,
endIndex: errorPosition + 1,
suggest: [
{
message: 'Add a space after the class_list filter',
fix(corrector) {
corrector.insert(errorPosition, ' ');
},
},
],
});
}
},
};
},
};
3 changes: 3 additions & 0 deletions packages/theme-check-node/configs/all.yml
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,9 @@ RemoteAsset:
RequiredLayoutThemeObject:
enabled: true
severity: 0
SpaceAfterClassList:
enabled: true
severity: 0
TranslationKeyExists:
enabled: true
severity: 0
Expand Down
3 changes: 3 additions & 0 deletions packages/theme-check-node/configs/recommended.yml
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,9 @@ RemoteAsset:
RequiredLayoutThemeObject:
enabled: true
severity: 0
SpaceAfterClassList:
enabled: true
severity: 0
TranslationKeyExists:
enabled: true
severity: 0
Expand Down
Loading