Skip to content

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

Merged
DanielRosenwasser merged 13 commits intomicrosoft:masterfrom
saschanaz:formatTsxAttr
Dec 30, 2015
Merged

Fixing JSX/TSX closing tag/attribute/expression formatting#4398
DanielRosenwasser merged 13 commits intomicrosoft:masterfrom
saschanaz:formatTsxAttr

Conversation

@saschanaz
Copy link
Copy Markdown
Contributor

@saschanaz saschanaz commented Aug 21, 2015

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>

mhegazy
mhegazy reviewed Aug 21, 2015
View reviewed changes
Comment thread src/services/formatting/formatting.ts Outdated
Copy link
Copy Markdown
Contributor

@mhegazy mhegazy Aug 21, 2015

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
Copy Markdown
Contributor

@mhegazy mhegazy Aug 21, 2015

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
Copy Markdown
Contributor

@mhegazy mhegazy Aug 21, 2015

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
Copy Markdown
Contributor Author

@saschanaz saschanaz Aug 25, 2015

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
Copy Markdown
Contributor Author

saschanaz commented Aug 24, 2015

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.

@saschanaz saschanaz mentioned this pull request Sep 2, 2015
Closed
@saschanaz saschanaz mentioned this pull request Sep 12, 2015
Merged
@kuon
Copy link
Copy Markdown

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
Copy Markdown
Contributor Author

saschanaz commented Sep 21, 2015

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

@saschanaz saschanaz mentioned this pull request Dec 4, 2015
Closed
@mhegazy
Copy link
Copy Markdown
Contributor

mhegazy commented Dec 9, 2015

👍

@mhegazy
Copy link
Copy Markdown
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.

@vladima vladima mentioned this pull request Dec 29, 2015
Closed
DanielRosenwasser added a commit that referenced this pull request Dec 30, 2015
@DanielRosenwasser
Merge pull request #4398 from SaschaNaz/formatTsxAttr
99b771c 
Fixing JSX/TSX closing tag/attribute/expression formatting
@DanielRosenwasser DanielRosenwasser merged commit 99b771c into microsoft:master Dec 30, 2015
@DanielRosenwasser
Copy link
Copy Markdown
Member

DanielRosenwasser commented Dec 30, 2015

Thanks @saschanaz!

This was referenced Jan 4, 2016
Merged
Closed
@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.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@saschanaz @kuon @mhegazy @DanielRosenwasser @msftclas