Space around class_list #418

geddski · 2024-07-15T21:23:10Z

What are you adding in this PR?

This theme check ensures that a space is used before and after the class_list filter. Otherwise the CSS classes before/after the filter will be concatenated to the class_list's generated class name, causing neither to work.

For example this theme check catches this problematic case:

<div class="{{ block.settings.spacing | class_list }}other-class">
    content
</div>

And this problematic case:

<div class="other-class{{ block.settings.spacing | class_list }}">
    content
</div>

Before you deploy

This PR includes a new checks or changes the configuration of a check
- I included a minor bump changeset
- It's in the allChecks array in src/checks/index.ts
- I ran yarn build and committed the updated configuration files

albchu

Just a few tweaks and questions but looking good!

packages/theme-check-common/src/checks/space-after-class_list/index.ts

albchu · 2024-07-16T04:02:29Z

packages/theme-check-common/src/checks/space-after-class_list/index.ts

+    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',


Is there another PR I can check out for this?

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

here we go: https://github.com/Shopify/shopify-dev/pull/46570

packages/theme-check-common/package.json

albchu

lgtm! Thanks Dave!

geddski · 2024-07-17T06:43:22Z

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.

albchu

LGTM. One minor doc change suggestion but then youre good to go! Thanks Dave :)

albchu · 2024-07-19T15:53:57Z

docs/contributing.md

@@ -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.


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`.

albchu · 2024-07-19T15:54:58Z

packages/theme-check-common/src/checks/space-around-class_list/index.ts

+              {
+                message: 'Add a space after the class_list filter',
+                fix(corrector) {
+                  corrector.insert(errorPosition, ' ');


karreiro

Thank you for this PR, @geddski! I've left only one minor suggestion about how we capture the absence of the space character. Please let me know what you think about that :)

karreiro · 2024-07-22T06:54:34Z

packages/theme-check-common/src/checks/space-around-class_list/index.ts

+          '';
+
+        // check for missing space after class_list
+        const afterRegex = /([a-zA-Z0-9._-]+)\s*\|\s*class_list\s*}}([a-zA-Z0-9._-]+)/gm;


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?

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?

Yes, that's my understanding as well :)

geddski added 3 commits July 12, 2024 14:44

add contributing note about the cache

120e0fd

space-after-class_list theme check

5a96df0

theme-check-common v 2.9.0

c976d9b

geddski requested a review from albchu July 15, 2024 21:23

fix formatting

9f611e1

geddski changed the title ~~Space-after-classlist~~ Space after class_list Jul 15, 2024

albchu requested changes Jul 16, 2024

View reviewed changes

geddski added 2 commits July 16, 2024 12:36

undo manual version bump

6e6394d

add changeset

5af3b5d

albchu approved these changes Jul 16, 2024

View reviewed changes

geddski marked this pull request as draft July 17, 2024 06:40

geddski added 2 commits July 18, 2024 16:05

rename to space-around-class_list

b3c9033

add check for missing space before class_list

95be3c4

geddski changed the title ~~Space after class_list~~ Space around class_list Jul 18, 2024

geddski marked this pull request as ready for review July 18, 2024 23:49

geddski requested a review from albchu July 18, 2024 23:50

albchu approved these changes Jul 19, 2024

View reviewed changes

karreiro reviewed Jul 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Space around class_list #418

Space around class_list #418

geddski commented Jul 15, 2024 •

edited

Loading

albchu left a comment

albchu Jul 16, 2024

geddski Jul 16, 2024

geddski Jul 16, 2024

albchu left a comment

geddski commented Jul 17, 2024

albchu left a comment

albchu Jul 19, 2024

albchu Jul 19, 2024

karreiro left a comment

karreiro Jul 22, 2024

geddski Jul 23, 2024

karreiro Jul 24, 2024

	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`.

Space around class_list #418

Are you sure you want to change the base?

Space around class_list #418

Conversation

geddski commented Jul 15, 2024 • edited Loading

What are you adding in this PR?

Before you deploy

albchu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albchu left a comment

Choose a reason for hiding this comment

geddski commented Jul 17, 2024

albchu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karreiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geddski commented Jul 15, 2024 •

edited

Loading