Space around class_list #418
Conversation
packages/theme-check-common/src/checks/space-after-class_list/index.ts
Outdated
Show resolved
Hide resolved
| 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', |
There was a problem hiding this comment.
Is there another PR I can check out for this?
There was a problem hiding this comment.
knew I was forgetting something! I'll see how to add that one sec
There was a problem hiding this comment.
|
Tyler had a great point that the same issue can happen with classes before class_list, so I'm going to update this theme check and its docs to account for that. |
| @@ -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. | |||
There was a problem hiding this comment.
Since this is general contribution docs, we should probably be a bit more specific about what this instruction is alluding to.
| 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`. |
| { | ||
| message: 'Add a space after the class_list filter', | ||
| fix(corrector) { | ||
| corrector.insert(errorPosition, ' '); |
| ''; | ||
|
|
||
| // check for missing space after class_list | ||
| const afterRegex = /([a-zA-Z0-9._-]+)\s*\|\s*class_list\s*}}([a-zA-Z0-9._-]+)/gm; |
There was a problem hiding this comment.
I believe we have valid cases, such as the following one where we want to raise an offense:
<div class="{{ block.settings.spacing | class_list }}{{ shop.name }}other-class">content</div>It's a bit changeling to cover that case relying on the regex tho. Instead, I believe we may navigate in the AST and can check if the sibling node starts with a space:
What do you think?
There was a problem hiding this comment.
ah, another interesting case. I'm glad y'all know your liquid! Would it be correct then to say the filter's next sibling (if any) must be a TextNode that starts with whitespace?
There was a problem hiding this comment.
Yes, that's my understanding as well :)
What are you adding in this PR?
This theme check ensures that a space is used before and after the
class_listfilter. Otherwise the CSS classes before/after the filter will be concatenated to theclass_list's generated class name, causing neither to work.For example this theme check catches this problematic case:
And this problematic case:
Before you deploy
changesetallChecksarray insrc/checks/index.tsyarn buildand committed the updated configuration files