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

turn off unicorn/template-indent #269

Merged
merged 32 commits into from
Dec 2, 2023
Merged

turn off unicorn/template-indent #269

merged 32 commits into from
Dec 2, 2023

Conversation

gurgunday
Copy link
Contributor

It conflicts with Prettier's template indenting

@lydell
Copy link
Member

lydell commented Nov 13, 2023

@gurgunday Can you show an example where there’s a conflict?

@gurgunday
Copy link
Contributor Author

Sure, here, Prettier is OK with this, but unicorn complains

Templates should be properly indented.eslint[unicorn/template-indent](https://github.com/sindresorhus/eslint-plugin-unicorn/blob/v49.0.0/docs/rules/template-indent.md)

const ShopItemPanelImage = (it) => {
  return html`
    <li class="carousel-item relative w-full" data-index="${it.i}">
      !${it.i === -1
        ? ""
        : html`
            <button
            >
              <svg
                xmlns="http://www.w3.org/2000/svg"
                width="24"
                height="24"
                viewBox="0 0 24 24"
                class="fill-current"
              >
                <title>Trash Icon</title>
                <path
                  d="M6 7H5v13a2 2 0 0 0 2 2h10a2 2 0 0 0 2-2V7H6zm10.618-3L15 2H9L7.382 4H3v2h18V4z"
                ></path>
              </svg>
            </button>
          `}
      <img
        onclick="uploadShopItemBanner(this)"
        src="${it.i === -1
          ? "/p/assets/img/shopBannerImage.png"
          : `${process.env.S3_CDN_URI}/userAssets/${it.user.id}/shop/${it.item.productID}/bannerPic${it.i}.webp`}"
        width="640"
        height="360"
        class="cursor-pointer !brightness-100 brightness-50 transition-[filter] duration-500 hover:!brightness-50"
        alt="${it.item.productName} banner image"
      />
    </li>
  `;
};

@JounQin
Copy link
Member

JounQin commented Nov 13, 2023

@lydell
Copy link
Member

lydell commented Nov 13, 2023

Thanks!

To me, the rule seems useful even with Prettier, as long as you configure the rule to not care about the same tags and comments as Prettier does. For example, the sql tag, the stripIndent function and the indent comment all sound useful with Prettier.

So maybe it should be marked as a special rule. It looks like the CLI tool might be able to validate the configuration, but I haven’t looked into it deeply.

@gurgunday
Copy link
Contributor Author

For example, the sql tag, the stripIndent function and the indent comment all sound useful with Prettier.

I can keep em all except html 😁

@gurgunday
Copy link
Contributor Author

I think it's better now

Not sure how a special rule works, does that just mean there should be documentation on what happens?

index.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@lydell lydell left a comment

Choose a reason for hiding this comment

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

What is your take on:

We should look into if it’s feasible to make a validator for it: f5530d3/bin/validators.js

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@gurgunday
Copy link
Contributor Author

Couldn't find weather Prettier formats templates using comments or not

So I only checked for tags

Is there a resource where I can find that information?

@lydell
Copy link
Member

lydell commented Nov 13, 2023

The only resource I know of is the source code that I linked to earlier: https://github.com/prettier/prettier/tree/main/src/language-js/embed

There you can see that there’s an /* HTML */ comment at least.

bin/validators.js Outdated Show resolved Hide resolved
bin/validators.js Outdated Show resolved Hide resolved
bin/validators.js Show resolved Hide resolved
bin/validators.js Outdated Show resolved Hide resolved
@gurgunday
Copy link
Contributor Author

I will check if there are other tag keywords

bin/validators.js Outdated Show resolved Hide resolved
@gurgunday
Copy link
Contributor Author

https://github.com/search?q=repo%3Aprettier%2Fprettier+tag.name&type=code

All of the tags seem to have been taken into account if I'm not missing anything

@lydell
Copy link
Member

lydell commented Nov 20, 2023

Looking good! I’m hoping to get some time for some testing on my own, merging and releasing soon!

bin/validators.js Outdated Show resolved Hide resolved
@lydell lydell merged commit 25fc427 into prettier:main Dec 2, 2023
10 checks passed
@lydell
Copy link
Member

lydell commented Dec 2, 2023

Released in v9.1.0. Thank you for the help! 💯

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