mxw / vim-jsx

React JSX syntax highlighting and indenting for vim.
1.59k stars 95 forks source link

Add support for multiline opening tags #75

Closed ksmithbaylor closed 8 years ago

ksmithbaylor commented 8 years ago

Fixes #74.

This (optionally) corrects the indentation when opening tags to span multiple lines. So instead of:

<Foo
  prop1="hey"
  prop2="there"
  >
  <Child />
</Foo>

The indentation will now be:

<Foo
  prop1="hey"
  prop2="there"
>
  <Child />
</Foo>

I imitated the way it's done for multiline self-closing tags. I put it behind a flag, but it may be the case that this should be the default.

Regardless of whether or not a user of this plugin prefers to keep the closing angle-bracket on the same line as the last prop or not, this change makes the use case of keeping it on a separate line behave as expected instead of being indented at the same level as the child components.

ksmithbaylor commented 8 years ago

After thinking about it more, it makes more sense to me to take this out from behind the flag. I can't think of a case where the first syntax would be desirable (assuming the placement of the closing angle bracket on its own line).

mxw commented 8 years ago

Oh huhn, I guess I did special case multiline self-closing tags...

I'll try to test this locally a bit before accepting (but I'm currently sick, so it might be a few days... please feel free to ping next week if you haven't heard from me).

ksmithbaylor commented 8 years ago

Thanks Max! Hope you feel better! I'll check back next week.

ksmithbaylor commented 8 years ago

Hey Max, did you get a chance to take a look?

mxw commented 8 years ago

(This looks good overall—just a bit of reworking to do before I pull it in.)

ksmithbaylor commented 8 years ago

Not quite! The second regex removes the \/ after the first s* in order to match a closing angle bracket without a leading /. Here are the regexes aligned so the difference is more obvious:

let s:sctag =            '^\s*\/>\s*;\='
let s:multilineopentag = '^\s*>\s*;\='
mxw commented 8 years ago

But, as these are regular expressions, can't you just make the \/ optional, with \/\?, as I suggested? Maybe I'm misreading something?

ksmithbaylor commented 8 years ago

Oh, you're right. Sorry! I combined them and changed the comments.

lencioni commented 8 years ago

I just tried out 6d32ac4 and it seems to be working as expected. Thanks for working on this @ksmithbaylor! @mxw is there anything left to be done before this can be merged?

mxw commented 8 years ago

It would probably help if I read my emails :]