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

Html: Enable jsxBracketSameLine option for html (fixes #5377) #9936

Closed
wants to merge 1 commit into from

Conversation

karptonite
Copy link

Description

This is an incomplete first pass at fixing #5377.

Essentially, it makes the jsxBracketSameLine option also apply in html files. Separately, this option should be renamed to reflect the new functionality; I suggest that angleBracketSameLine is a sensible name.

As you can see in the tests, when jsxBracketSameLine is set, this will convert:

<article
  id="electriccars"
  data-columns="3"
  data-index-number="12314"
  data-parent="cars">...</article>
<span
id="electriccars"
  data-columns="3"
  data-index-number="12314"
  data-parent="cars">...</span>

to

<article
  id="electriccars"
  data-columns="3"
  data-index-number="12314"
  data-parent="cars">
  ...
</article>
<span
  id="electriccars"
  data-columns="3"
  data-index-number="12314"
  data-parent="cars"
  >...</span
>

Without that setting, this would look like:

<article
  id="electriccars"
  data-columns="3"
  data-index-number="12314"
  data-parent="cars"
>
  ...
</article>
<span
  id="electriccars"
  data-columns="3"
  data-index-number="12314"
  data-parent="cars"
  >...</span
>

Note that because of the whitespace sensitivity of a span, the span is formatted the same way for each option.
Also note that nothing is done with the final > in the closing span tag. This isn't something that comes up with jsx, as I understand it, because of how jsx handles whitespace. I'm not sure what the desirable behavior should be here, but if the setting is called angleBracketSameLine, perhaps that should be brought up to the previous line as well. But I'm not sure how to accomplish that, if that is the desired behavior.

Thanks to @adamJLev for pointing me in the right direction regarding this fix.

While I have added a test for the above cases, I have no idea what additional test might be desirable. I'm happy to work with someone from the Prettier team if more tests are needed.

I also haven't touched the docs yet. I assume that would come with a change to the name of the new option, or possibly renaming the old option to reflect both options, but that seems like a much larger job.

Checklist

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory).
  • (If the change is user-facing) I’ve added my changes to changelog_unreleased/*/XXXX.md file following changelog_unreleased/TEMPLATE.md.
  • I’ve read the contributing guidelines.

Try the playground for this PR

@@ -0,0 +1,2 @@
run_spec(__dirname, ["html"], { jsxBracketSameLine: true });
run_spec(__dirname, ["html"], { jsxBracketSameLine: false });
Copy link
Member

Choose a reason for hiding this comment

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

It should be bracketSameLine or other name, not jsxBracketSameLine, because jsx is not html, I would advise to do:

First PR.

  • Added the new option
  • if developer doesn't set jsxBracketSameLine we will try value of bracketSameLine, if still nothing, use default value

Second PR.

  • Improve this for HTML + tests

Third PR.

  • Improve this for VUE and etc

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, I addressed the naming issue in my post above. The eventual goal is that there is only one option, right? with jsxBracketSameLine being either a synonym of angleBracketSameLine (or whatever it is called) or deprecated.

Copy link
Member

Choose a reason for hiding this comment

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

/cc @prettier/core

Copy link
Member

Choose a reason for hiding this comment

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

We need renamejsxBracketSameLine first. Then apply to html .

Copy link
Author

Choose a reason for hiding this comment

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

Let me look into a different pull request that starts with a rename.

Copy link
Author

Choose a reason for hiding this comment

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

Would a pull request for a rename include renaming in all the tests and docs? Should there be references to the old name in the docs? What section does the new option go in, now that it applies to both javascript AND html? Just trying to figure out what I need to submit here to get the ball rolling.

also, @alexander-akait , you wrote:

if developer doesn't set jsxBracketSameLine we will try value of bracketSameLine, if still nothing, use default value

I think we could simplify that to "If a developer sets either of the settings to true, we print the bracket on the same line." The default is false, and we don't want to inadvertently create a new option here by treating jsxBracketSameLine differently from angleBracketSameLine. We are renaming an old option, not adding a new one.

@dummdidumm
Copy link

dummdidumm commented Jan 7, 2021

Note that because of the whitespace sensitivity of a span, the span is formatted the same way for each option.
Also note that nothing is done with the final > in the closing span tag. This isn't something that comes up with jsx, as I understand it, because of how jsx handles whitespace. I'm not sure what the desirable behavior should be here, but if the setting is called angleBracketSameLine, perhaps that should be brought up to the previous line as well. But I'm not sure how to accomplish that, if that is the desired behavior.

If it's done for the > of the closing tag aswell, I think it should only be done if there's whitespace after the >. In the following example this would mean only the last closing tag is on the same line:

Input

<span><span>text</span>letspretendthisbreaks<span>hi</span></span>

Output

<span
    ><span>text</span
    >letspretendthisbreaks<span
        >hi</span
    ></span>

Base automatically changed from master to main January 23, 2021 17:13
@alexander-akait
Copy link
Member

#11006

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants