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

Starting point of indent for @html-eslint/indent with HTML Javascript template literals #241

Closed
jpradelle opened this issue Dec 3, 2024 · 8 comments · Fixed by #242 or #247
Closed

Comments

@jpradelle
Copy link

When using javascript HTML literals, it always requests to start at 0 for top level tag, do you think it could be updated to handle cases like this:

import {html, LitElement} from 'lit';

export class TestTest extends LitElement {
  render() {
    return html`
      <p>Expected indent to start like that, at 6</p>
    `;
  }
}
customElements.define('test-test', TestTest);

Current output is

test-test.js
  8:1  error  Expected indentation of 0 space but found 6 space  @html-eslint/indent

I also see a bit more complex cases like that:

import {html, LitElement} from 'lit';
import {repeat} from 'lit/directives/repeat.js';

export class TestTest extends LitElement {
  render() {
    return html`
      <p>Expected indent to start like that, at 6</p>
      ${repeat([], item => html`
        <p>Expected indent to start like that, at 8</p>
      `)}
      ${repeat(
          [],
          item => html`
            <p>Expected indent to start like that, at 12</p>
          `)}
      ${
        repeat(
            [],
            item => html`
              <p>Expected indent to start like that, at 12</p>
            `)
      }
    `;
  }
}
customElements.define('test-test', TestTest);

I think we would like initial indent reference to be something like start position of 1st non indent char of line containing html` + 1 indent level.

@yeonjuan
Copy link
Owner

yeonjuan commented Dec 3, 2024

Hi @jpradelle Thanks for the reporting this issue.

I think we would like initial indent reference to be something like start position of 1st non indent char of line containing html

This seems reasonable. let me fix it!

@yeonjuan
Copy link
Owner

yeonjuan commented Dec 3, 2024

Hi @jpradelle, It's fixed in v0.30.0 👍

@jpradelle
Copy link
Author

Thanks a lot !

That changes a bit my habits, I usually write

import {LitElement, html} from 'lit';
import {when} from 'lit/directives/when.js';

export class TestTest extends LitElement {
  render() {
    return html`
      <p>test1</p>
      ${when(true, () => html`
        <p>test2</p>
      `)}
      <p>test3</p>
    `;
  }
}
customElements.define('test-test', TestTest);

It's requesting:

import {LitElement, html} from 'lit';
import {when} from 'lit/directives/when.js';

export class TestTest extends LitElement {
  render() {
    return html`
      <p>test1</p>
      ${when(true, () => html`
          <p>test2</p>
        `)}
      <p>test3</p>
    `;
  }
}
customElements.define('test-test', TestTest);

With 2 levels for <p>test2</p>. As we are in function parameters, not sure which starting point should be the reference.
Even lit.dev playground is not consistant: https://lit.dev/playground/#sample=examples/directive-repeat and https://lit.dev/playground/#sample=examples/properties-custom-converter and https://lit.dev/playground/#sample=examples/templates-conditional

My IDE tries to push me to do the way you did it too.

I also have all multi line attributes indented with 2 levels of indent, for exemple

<foo-bar
    attr1
    attr2="foo"></foo-bar>

Linter requests only one level, do you think you could add an option so we can request 2 indent levels for multiple lines tags ?

That follows 2nd and 3rd sample of google guidelines: https://google.github.io/styleguide/htmlcssguide.html#HTML_Line-Wrapping

@yeonjuan
Copy link
Owner

yeonjuan commented Dec 4, 2024

@jpradelle It would be nice to have separate options for the four attributes. I've created an issue.
#244

@yeonjuan
Copy link
Owner

yeonjuan commented Dec 10, 2024

@jpradelle
Copy link
Author

Hi thanks a lot, works exactly as expected for attributes.

About previous comment on template literal root base indent, sorry to bother you again do you think it could be updated or an option could be added:

On this example:

import {LitElement, html} from 'lit';
import {when} from 'lit/directives/when.js';

export class TestTest extends LitElement {
  render() {
    return html`
      ${when(true, () => html`
        <foo-bar
            .baz="${value => html`
              <foo-baz
                  bar></foo-baz>
            `}"></foo-bar>
      `)}
    `;
  }
}
customElements.define('test-test', TestTest);

Report is

test-test.js
   8:1  error  Expected indentation of 10 space but found 8 space   @html-eslint/indent
   9:1  error  Expected indentation of 14 space but found 12 space  @html-eslint/indent
  10:1  error  Expected indentation of 22 space but found 14 space  @html-eslint/indent
  11:1  error  Expected indentation of 26 space but found 18 space  @html-eslint/indent

Which means fixed version is

import {LitElement, html} from 'lit';
import {when} from 'lit/directives/when.js';

export class TestTest extends LitElement {
  render() {
    return html`
      ${when(true, () => html`
          <foo-bar
              .baz="${value => html`
                        <foo-baz
                            bar></foo-baz>
              `}"></foo-bar>
      `)}
    `;
  }
}
customElements.define('test-test', TestTest);

Indent base reference seems to be the opening curly brace

              .baz="${value => html`
                      ^
                      |

Could it be base indent of line on which template opening is

              .baz="${value => html`
              ^
              |

I think both can make sense, but currently in some complex cases, indent can increase a lot very fast

@yeonjuan yeonjuan reopened this Dec 11, 2024
@yeonjuan
Copy link
Owner

yeonjuan commented Dec 11, 2024

@jpradelle oops!. In the current version, when tempalte literals are nested, the indents keep stacking up. This seems to be the cause, and I've raised a PR to fix it. #247
It has also been prereleased. (0.31.1-alpha.0)
I'd appreciate it if you could check it out with the pre-release version.

  • don't need to upgrade parser version. only eslint-plugin published
npm install -D @html-eslint/eslint-plugin@0.31.1-alpha.0

@jpradelle
Copy link
Author

Awesome, works exactly as expected: applying this on my project reported no false positive, and it reported few issues as expected :)
Thank you so much !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants