Skip to content

Fixing JSX/TSX closing tag/attribute/expression formatting #4398

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

Merged
merged 13 commits into from
Dec 30, 2015

Conversation

saschanaz
Copy link
Contributor

Closes #3838.

Current:

let x = <div>
<div>
    </div>
    </div>

let y = <div
    className="foo"
    id={
    3
    }>
</div>

Fix:

let x = <div>
    <div>
    </div>
</div>

let y = <div
    className="foo"
    id={
        3
    }>
</div>

@@ -387,7 +389,10 @@ namespace ts.formatting {

let indentation = inheritedIndentation;
if (indentation === Constants.Unknown) {
if (isSomeBlock(node.kind)) {
if (isIndentPreventedChildNode(parent.kind, node.kind)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

i would inline the body of isIndentPreventedChildNode,

if (parent.kind === SyntaxKind.JsxElement && child.kind === SyntaxKind.JsxClosingElement)

it is more readable this way.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is similar to closing braces and parens, maybe we should add it to get indentationForToken.

Copy link
Contributor

Choose a reason for hiding this comment

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

possibly move getIndentationForToken to a new helper, call it in getIndentationForToken, and from here and add support for jsx elements in it as well.. possibly give the new helper a better name as getIndentationForNode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mhegazy I currently have no idea so I just inlined it. I agree that the closing node is similar to closing tokens but I think nodes and tokens are being processed with some differences by different functions so a merged helper may not really simplify the code.

@saschanaz
Copy link
Contributor Author

I have to figure out how I can merge getIndentationForToken and isIndentPreventedChildNode. A simple merge breaks indentation here:

let x = <div>
</div>; // works well

let y = foo
(3); // should be indented but fails

PS: I've decided not to do this at least in this PR.

@kuon
Copy link

kuon commented Sep 21, 2015

Will this cover the following (parenthesis and newline)?

return (
    <div>
        <div />
    </div>
);

If yes, perhaps adding a test for this specific case.

@saschanaz
Copy link
Contributor Author

@kuon Yes it does, and I think this PR already tests that case (where JsxElement is contained in parenthesis.)

@mhegazy
Copy link
Contributor

mhegazy commented Dec 9, 2015

👍

@mhegazy
Copy link
Contributor

mhegazy commented Dec 9, 2015

can you please refresh this PR, i think we are ready to go. Thanks for your patience and sorry for the delay.

DanielRosenwasser added a commit that referenced this pull request Dec 30, 2015
Fixing JSX/TSX closing tag/attribute/expression formatting
@DanielRosenwasser DanielRosenwasser merged commit 99b771c into microsoft:master Dec 30, 2015
@DanielRosenwasser
Copy link
Member

Thanks @saschanaz!

@saschanaz saschanaz deleted the formatTsxAttr branch January 29, 2016 02:20
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
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.

5 participants