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

Feature idea: easy classNames #6

Closed
staltz opened this issue Oct 13, 2015 · 15 comments
Closed

Feature idea: easy classNames #6

staltz opened this issue Oct 13, 2015 · 15 comments

Comments

@staltz
Copy link
Contributor

staltz commented Oct 13, 2015

One feature I like in hyperscript is the ability to easily write the className using CSS selectors:

h('div.foo'), not h('div', {className: 'foo'}).

In Cycle.js, it's also convenient to have the . dot there, because later the classes are used in selectors, e.g. DOM.select('.foo').events('click'). So it's not great if sometimes you need the dot, and sometimes you don't (in className), that's why I like hyperscript keeping it consistent.

Proposal:
How about allowing a string parameter to the hyperscript helper? If the first parameter is a string, then it's interpreted as the selector, so its classes will be parsed out in hyperscript. If the first parameter is an object, we know its the properties object.

E.g.
div('.foo') instead of div({className: 'foo'}).

Of course one would be able to still provide properties and children:

div('.foo', {'data-id': 'headline-6.1.2'}, [
  img({src: user.avatar})
])
@ohanhi
Copy link
Owner

ohanhi commented Oct 13, 2015

@staltz Well, if it's just about matching the .<className>, it should be fairly straightforward. Do you think #<id> should be added too, for at least some level of "completeness"?

@staltz
Copy link
Contributor Author

staltz commented Oct 13, 2015

Yes, because we can use the underlying logic already existing in hyperscript.

Instead of calling h(tagName, ...argsArray) we call h(tagName + selector, ...argsArray).

@ohanhi
Copy link
Owner

ohanhi commented Oct 13, 2015

Could it really be that easy? I thought there was some mystical logic in hyperscript that prevented me from doing that in the first place... No, it was the "figure out whether the first parameter is a selector and maybe act differently" part that felt icky to me. But I do see the value in this proposal.

@Frikki
Copy link

Frikki commented Oct 13, 2015

Can I give my 👍 for this?

@ohanhi ohanhi closed this as completed in a1ca4ec Oct 13, 2015
@ohanhi
Copy link
Owner

ohanhi commented Oct 13, 2015

@staltz, @Frikki, published to npm as 2.0.0.

@Frikki
Copy link

Frikki commented Oct 13, 2015

Sweet! 👯

@ivan-kleshnin
Copy link
Contributor

Just wanted to note that this shortcut may be a source of confusion.

title("...")

Will generate

<title class=".."/>

instead of

<title>...</title>

@staltz
Copy link
Contributor Author

staltz commented Dec 8, 2015

Hmm, good find

@ivan-kleshnin
Copy link
Contributor

I have code like title(ctx.seotitle || "...") in my pseudo-templates and discovered this.
It's trivial to overcome with title([ctx.seotitle || "..."]) but may be worth mentioning in README.

@ohanhi
Copy link
Owner

ohanhi commented Dec 9, 2015

@ivan-kleshnin Do you think it is enough to mention this in README or should this be resolved somehow?

We could theoretically make the "class name detection" smarter by also checking that the 2nd character is [a-zA-Z], or whatever is in the CSS classname specs.

@ivan-kleshnin
Copy link
Contributor

We could theoretically make the "class name detection" smarter by also checking that the 2nd character is [a-zA-Z], or whatever is in the CSS classname specs.

@ohanhi yes we could. But I personally prefer to avoid heuristics because they kinda hide the problem under the carpet. For example this check could prevent

renderBody({title: "..."}) -> ... -> ... -> ... -> ... title(seotitle)

surprising output. But it couldn't prevent [span("README.md"), span(".gitignore")] occasional "bug".

I like the API of hyperscript-helpers and I'd like to keep this shortcut. It's very convenient.

So in my opinion we should just mention this in docs. Something like

If you pass a variable which can start with . as a first argument to vnode, always wrap it in [].
For example: span(filename) should be written as span([filename]) to render filenames like .gitignore appropriately.

@ohanhi
Copy link
Owner

ohanhi commented Dec 9, 2015

👍 Would you like to make a PR about that?

@ivan-kleshnin
Copy link
Contributor

Sure. One more alternative is to require from single argument to be an Array.
I.e. forbid title("xxx") calls alltogether and allow only title(["xxx"]) and title({}, ["xxx"]) versions. But this is backward incompatible and still may cause questions.

@ohanhi
Copy link
Owner

ohanhi commented Dec 9, 2015

Yes, but I don't think we need to go that far. The gist of this library is anyway to be a simple proxy for the hyperscript implementation. Introducing any syntax rules of our own easily makes the lib less convenient to use and maintain. So I think just the note in README is the best way to go about this.

@ivan-kleshnin
Copy link
Contributor

Sent a PR. I opened you a push access to fork for case you'll want to change something.

ivan-kleshnin added a commit to ivan-kleshnin/html-to-hyperscript that referenced this issue Dec 31, 2015
For HH syntax, even if node has single text child,
it should be wrapped in bracked due to ohanhi/hyperscript-helpers#6 (comment)

For HH syntax, hovewer it is not required.

Brackets around single commend child are not required in both syntaxes.
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

No branches or pull requests

4 participants