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

Group HyperScript tag #6330

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Group HyperScript tag #6330

wants to merge 1 commit into from

Conversation

nykula
Copy link
Contributor

@nykula nykula commented Jul 26, 2019

  • I’ve added tests to confirm my change works.
  • (If the change is user-facing) I’ve added my changes to the CHANGELOG.unreleased.md file following the template.
  • I’ve read the contributing guidelines.

Try the playground for this PR

@nykula
Copy link
Contributor Author

nykula commented Aug 11, 2019

@lydell @streamich @tusharmath How do I keep HyperScript curly brace or short props inline? My patch groups the tag, but you might have a better solution, as you've talked about this earlier.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

/cc @prettier/core

@streamich
Copy link

@nykula I would try to do it same as in JSX—if there are few attributes, keep them on one line.

<div className={className}>Hello</div>

would be

h('div', {className}, 'Hello')

and

<div
  {...rest}
  style={style}
  className={(rest.className || '') + className}
>
  {children}
</div>

would be

h('div', {
  ...rest,
  style,
  className: (rest.className || '') + className,
},
  children
)

@nykula
Copy link
Contributor Author

nykula commented Aug 19, 2019

@streamich Yes matching JSX compactness is the goal. I did figure out how to stick the function name and tag together. Where I'm stuck is how to technically get { on the same line. Prettier has a group tool. Lets one treat multiple nodes in a sequence independent of their siblings, as a separate sequence, making it possible to keep short literal tokens in one line and other tokens multiline. However the {, unlike the function name and tag, is not an individual node, but a part of an object literal which is a node in its entirety. Prettier forces all nodes in a sequence onto split lines when they don't all fit in one line, even when they consitute a group independent of other nodes. When I add the object literal to the group, it outputs AFAIR either one of these no matter what solution I try:

h(
'div',
{
  ...rest,
  style,
  className: (rest.className || '') + className,
},

  children
);
h('div',
  {
    ...rest,
    style,
    className: (rest.className || '') + className,
  },
  children
);

I kept the second so far. Would be nice if anyone reading this cloned the pull request code and played with trying to get the { inline where I have the 'div', but leave the rest of the object split with newlines and no indent, without resulting in that extra newline at the end, thus replicating your example.

@j-f1
Copy link
Member

j-f1 commented Aug 19, 2019

You could put just the children in the group, something like this:

concat([
  'h(',
  name,
  ', ',
  props,
  group(indent(concat([hardline, ...children]))),
  hardline,
  ')',
])

@PascalPixel
Copy link

This is a good start, perhaps try to get the first and second arguments of a function on one line if they are <80 width combined, separately of 3rd or 4th etc argument length?

@nykula
Copy link
Contributor Author

nykula commented Sep 24, 2020

@PascalPixel It'd be great if you could try implementing that, maybe in a new pull request. For the HyperScript code I've been writing more recently, I apply a different tool, clang-format -style=Google, which behaves like you suggest.

Base automatically changed from master to main January 23, 2021 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants