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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/language-html/printer-html.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ const {
isVueSfcBindingsAttribute,
isScriptLikeTag,
isTextLikeNode,
isWhitespaceSensitiveNode,
preferHardlineAsLeadingSpaces,
shouldNotPrintClosingTag,
shouldPreserveContent,
Expand Down Expand Up @@ -728,7 +729,13 @@ function printAttributes(path, options, print) {
) {
parts.push(node.isSelfClosing ? " " : "");
} else {
parts.push(node.isSelfClosing ? line : softline);
const sensitiveEnd = node.isSelfClosing ? line : softline;
const insensitiveEnd = node.isSelfClosing ? " " : "";
parts.push(
options.jsxBracketSameLine && !isWhitespaceSensitiveNode(node)
? insensitiveEnd
: sensitiveEnd
);
}

return concat(parts);
Expand Down
76 changes: 76 additions & 0 deletions tests/html/last_line/__snapshots__/jsfmt.spec.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`last_line.html - {"jsxBracketSameLine":false} format 1`] = `
====================================options=====================================
jsxBracketSameLine: false
parsers: ["html"]
printWidth: 80
| printWidth
=====================================input======================================
<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>

=====================================output=====================================
<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
>

================================================================================
`;

exports[`last_line.html - {"jsxBracketSameLine":true} format 1`] = `
====================================options=====================================
jsxBracketSameLine: true
parsers: ["html"]
printWidth: 80
| printWidth
=====================================input======================================
<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>

=====================================output=====================================
<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
>

================================================================================
`;
2 changes: 2 additions & 0 deletions tests/html/last_line/jsfmt.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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

@karptonite karptonite Dec 23, 2020 •
edited
Loading

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.

10 changes: 10 additions & 0 deletions tests/html/last_line/last_line.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<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>