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

Update contentFor tag grammar to properly extract arguments #648

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

veken1199
Copy link
Contributor

@veken1199 veken1199 commented Dec 4, 2024

What are you adding in this PR?

It seems that the grammar we had for content_for did not properly extract the contentFor liquid tag argument and convert them into array.

In this PR we have:

  1. Updated the grammar for contentFor liquid tag to support arguments that contain .
  2. Updated ValidContentForArguments check to support closest. instead of context.
  3. Introduced a new DeprecatedContentForArgument check that reports a warning when contentFor tag contains context.
  • 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
      • If applicable, I've updated the theme-app-extension.yml config
  • I included a minor bump changeset
  • My feature is backward compatible
  • I included a patch bump changeset

@veken1199 veken1199 force-pushed the feken/update-fix-content-for-grammer branch from af44837 to 0e525f9 Compare December 4, 2024 14:52
@veken1199 veken1199 force-pushed the feken/update-fix-content-for-grammer branch 2 times, most recently from a27fb8e to 36d5bc7 Compare December 4, 2024 17:56
@veken1199 veken1199 requested a review from charlespwd December 4, 2024 18:11
@veken1199 veken1199 self-assigned this Dec 4, 2024
@veken1199 veken1199 force-pushed the feken/update-fix-content-for-grammer branch from 36d5bc7 to 6731fac Compare December 4, 2024 18:12
Copy link
Contributor

@albchu albchu left a comment

Choose a reason for hiding this comment

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

LGTM!

@veken1199 veken1199 marked this pull request as ready for review December 4, 2024 18:31
@veken1199 veken1199 force-pushed the feken/update-fix-content-for-grammer branch from 6731fac to 7f959f6 Compare December 4, 2024 18:39
Copy link
Contributor

@charlespwd charlespwd left a comment

Choose a reason for hiding this comment

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

Other than the nitpick, this LGTM :) Feel free to merge after!

@veken1199 veken1199 force-pushed the feken/update-fix-content-for-grammer branch from 7f959f6 to 8912fab Compare December 4, 2024 19:53
@veken1199 veken1199 merged commit 7e1df75 into main Dec 4, 2024
6 checks passed
@veken1199 veken1199 deleted the feken/update-fix-content-for-grammer branch December 4, 2024 20:02
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.

3 participants