seed-rs / seed

A Rust framework for creating web apps
MIT License
3.8k stars 153 forks source link

class! improvements #375

Closed MartinKavik closed 4 years ago

MartinKavik commented 4 years ago

Suggested class! changes:


Note: We need to write if_ function.

MuhannadAlrusayni commented 4 years ago

I wouldn't vote for renaming to C! since we use this macro only once per node. I would agree for the other points.

flosse commented 4 years ago

Suggested class! changes:

  • Rename class! to C! to improve readability and make it consistent with A, S, E, and T.

:-1:

  • Accepts also String and other text types.

:+1:

  • Don't write empty class to HTML, i.e. class![] shouldn't be rendered as <div class>.

:+1:

MartinKavik commented 4 years ago

I have problems to read (longer) view's because class! looks like element macros so it takes me a while to decide if the given element has children or it has only class defined. The same for finding class attribute among other entities. When I tried some options in source code, C! seems to me the most recognizable.

Counter arguments / other opinions or ideas?

flosse commented 4 years ago

Styles are key-value pairs for the attribute style (<div style="name:property;" />) so it makes sense to introduce a separate type S beside A. Event handlers are also special, so I can see why there is an E. But class is just one of hundreds of attribute names (like id or so). So I would not expect a separate C! macro. Of course it's a popular attribute so providing a convenient way to write it makes sense. But that's another bucket. Otherwise I would expect a I! for the popular id attribute, X! and Y! for the popular attributes x and y (in svg images), etc.

MartinKavik commented 4 years ago

I would rather use A.class("..") & A.id("..") and get rid of class! and id!. However I haven't found a reasonable way how to express class conditionals class!["active" => active] in A.class(..) and remove verbosity caused by multiple items / classes.

MartinKavik commented 4 years ago

But class is just one of hundreds of attribute names (like id or so). So I would not expect a separate C! macro.

div![
    id!("submit-btn"),
    class!["btn", "btn-primary", &my_classes, "active" => active],
    "Submit",
]

vs

div![
    A.id("submit-btn"),
    A.class(["btn", "btn-primary", &my_class, if_(active, "active")]),
    "Submit",
]

Note: if_ would we usable also with elements, event handlers, etc. - ie. if_(cond, div![]) ~ if cond { div![] } else { empty![] }.

What do you think?

MuhannadAlrusayni commented 4 years ago

I haven't found a reasonable way how to express class conditionals class!["active" => active]

Another way to do it with function class(): https://gist.github.com/MuhannadAlrusayni/dd291089cb7f92510a20c80be81fa58b

Usage:

let attrs = vec![
    class("foo"),
    class("foo".to_string()),
    class(Some("foo")),
    class(vec!["foo", "boo"]),
    class(("foo_true", true)),
    class(("foo_false", false)),
    class(vec![Some("foo"), None, Some("boo")]),
];
// [Class(["foo"]), Class(["foo"]), Class(["foo"]), Class(["foo", "boo"]), Class(["foo_true"]), Class([]), Class(["foo", "boo"])]
println!("{:?}", attrs);
MartinKavik commented 4 years ago

OT: @MuhannadAlrusayni please use Gist or Rust playground for long code snippets.

TatriX commented 4 years ago

I wonder whether it's possible to do something like this:

node![div, ".foo.bar", ...]
// or maybe
node![div, ["foo", "bar" => active], "Text", span!["Hi"]]
MuhannadAlrusayni commented 4 years ago

@TatriX I believe we can achieve this, but might not be more readable than what we have now, imagine something like this:

node![
    div, .foo #foo#bar
    node![
        h1, .page-title #title,
        "Page Title"
    ],
]

vs

div![
    class!("foo", "bar"),
    id!("foo"),
    h1![
        class!("title"),
        id!("page-title"),
        "Page Title"
    ],
]
flosse commented 4 years ago

BTW: on the backend I really like to use maud, so I'd love to use it on the frontend too :)

div #submit-btn .btn .btn-primary class=(&my_class) class="active"?[active] {
  "Submit"
}

vs

div![
    A.id("submit-btn"),
    A.class(["btn", "btn-primary", &my_class, if_(active, "active")]),
    "Submit",
]
MartinKavik commented 4 years ago
rebo commented 4 years ago

So in my opinion the key thing for Seed's success has to be developer experience over and above all other considerations bar correctness and sensible performance.

My concern about the below is that it's very noisy, messy and annoying to write out every single time I want a class (which I almost always do for every element):

A.class(["btn", "btn-primary", &my_class, if_(active, "active")]),

Whilst it is conceptually and structurally true that class is just another attribute it is pretty much the most commonly used one. Therefore, from the developer experience perspective there is a reason to special case it.

Think about a developer experienced in JSX they are used to something simple like <div className = "btn"> we dont want to raise the barrier to new developers because they might look at all the extra syntax and nope out.

Personally I think

C!["x", "y", &my_class, if_(active, "active")];

is far more readable and maintainable than this:

A.class(["btn", "btn-primary", &my_class, if_(active, "active")]),

I think the C! version is far easier to scan so I can pick out the relevant classes.

So my solution is to allow both. A.class ... would be allowed precisely because class is 'just another attribute' but C! is effectively syntactic sugar to clean this up.

I definitely agree with using if_ though, it reads correctly in terms of english as well.

 if_(active, "active")

if active is true then use the "active" class.

TatriX commented 4 years ago

I think if_ looks a bit ugly, so maybe when/unless instead?

div![
    c!["x", "y", &my_class, when(active, "active")],
    unless(hidden, span!["xxx"]),
]
TatriX commented 4 years ago

Here's another option with several different types of syntax for attributes:

div![
     class => ["x", "y", &my_class, when(active, "active")],
     id = "foo",
     :style ["float": "left"],
     onclick: |_| Msg::Click,
     "Text here",
     span!["quux"],
];

Tag macro can contain either impl UpdateEl or attribute_name DELIMITER $value.

TatriX commented 4 years ago

Personally I like class: ["foo"] syntax.

MartinKavik commented 4 years ago

I think if_ looks a bit ugly, so maybe when/unless instead?

class: ["foo", when(active, "bar")], id: "quux",

I agree it's very readable, but autocomplete isn't applicable at all and it can get annoying when you want to use attributes with enum of values, e.g. autocomplete: Autocomplete::Off (verbose and you have to import enum) or autocomplete: off (is off a variable or a variant?). Perhaps I would rather choose existing template system like @flosse suggested instead of it. And element macros would become more "magical".

rebo commented 4 years ago

Thumbs up for when(condition, attribute).

flosse commented 4 years ago

I'm sorry for the late response

  • We can't use maud now because it requires nightly.

yes, that's right.

  • I can imagine it would scare beginners - e.g. React developers would have to learn TEA, Rust, Maud and Seed API at once.

I think it's a bit scary anyway and there is not much difference between learning macros of seed or maud; both are kind of a DSL.

  • Is it possible to handle special things like element references or element keys with Maud?

no, I don't think so.

  • Both examples ^ seems pretty comparable to me, but I see your point - you can share the same template system with the backend and eyes are able to quickly identify symbols like # and . as ids and classes because we are used to see them often.

It's not better or worse, it's possible just a personal preference.

  • I started to work on Seed because I was annoyed by Yew's html! macro - very cryptic errors, need to learn another language, not flexible enough..

I see.

  • I'm afraid it'll make Seed code-base more complex and it would be another dependency and also key dependency without our direct control.

Absolutely. I didn't mean to actually depend on maud but it's probably an interesting inspiration.

  • I don't know about Maud IDE support, but I'm afraid it will be always worse than for Rust's native syntax.

I agree!

  • Is it possible to transform Maud template to Seed "template"? E.g. Does Maud have some parsers / convertors / plugins for it?

Oh, I don't think so.

So again, my comment was meant to be an inspiration :)

flosse commented 4 years ago

That's my personal ranking for conditional expressions:

  1. when(not(active), "active")
  2. when(!active, "active")
  3. if_(!active, "active")
  4. unless(active, "active") ... I also remember the CoffeeScript's unless bugs, so let's avoid it!
MartinKavik commented 4 years ago

Implemented in https://github.com/seed-rs/seed/pull/390

sabine commented 4 years ago

I'm fine with the semantics of C! as it is now, but I believe in discoverability. As a new developer coming to Seed, I shouldn't have to guess and look up what C! means.

When it's named class!, the likelihood of a newcomer having a great onboarding experience is increased, because HTML looks so similar. Ideally someone familiar with either Elm, React, Angular should be able to read and understand the TodoMVC example with only minimal commentary.

Edit: there's nothing wrong with offering C! or similar shorthands for personal use. Maybe at some point, someone will implement JSX on top of Seed for their personal use (and for everyone who wants to opt into that), that's fine. Still, I believe that having speaking names for a great onboarding experience is crucial. Reduce cognitive load in examples as much as possible.