withClass should append class, not replace

[jjYBdx4IL]

That would simplify code in quite a few cases.

[tipsy]

Can you show me your code? The more idiomatic way to write it would probably be tag.withClasses("a", "b", iff(true, "c"))

[jjYBdx4IL]

it's just that one sometimes has to add classes to the same element at different positions in the code, and replacing existing class names is practically never needed. a separate "addClass" method would be okay too.

[h2software]

+1
Example usage
if (isActive) { tag = tag.appendClass("active") }

[tipsy]

@h2software can you show me a bigger code example where you want to use this? I understand the simple cases, but I really think you should use tag.withClasses("nav", iff(isActive, "active")) for those.

[h2software]

Sure...

            ContainerTag feedbackDiv = div();
            boolean hasFeedback = false;
            if (!feedback.isAnswerBlank(exerciseInput)) {
                FeedbackStatus feedbackStatus = feedback.getFeedbackStatus(exerciseInput, key);
                if (feedbackStatus != null) {
                    FeedbackType feedbackType = feedbackStatus.getFeedbackType();
                    // There is no appendClass, so we're adding form-control AGAIN
                    tag = tag.withClass("form-control " + feedbackType.getClassForFormControl());
                    hasFeedback = feedbackStatus.getFeedbackText() != null;
                    feedbackDiv = feedbackDiv.with(text(feedbackStatus.getFeedbackText())).withClass(feedbackType.getClassForFeedbackDiv());
                }
            }

[tipsy]

@h2software there isn't a lot of j2html in your example, but I understand why you want the method. I really want to nudge people towards a more declarative programming style when using j2html though (similar to how you would write actual HTML).

There are currently no methods in j2html that append anything, so I'm reluctant to introduce this. It would be inconsistent, and it would encourage people to write very non-idiomatic j2html-code.

I'd be happy to assist you in converting your code to idiomatic j2html if you're interested.

[h2software]

No problem. Thank you for giving it some thought.
If withClass would append rather than replace, that would be consistent with your other with methods, though. For example tr().with(td()).with(td()) would add two data cells, not one.

[tipsy]

If withClass would append rather than replace, that would be consistent with your other with methods, though. For example tr().with(td()).with(td()) would add two data cells, not one.

Well, damn. That's a valid point, and a major oversight on my part. Unfortunately, this makes me want to change the behavior of ContainerTag#with, not the other way around. Are you also calling ContainerTag#with more than once per element?

[h2software]

Yes, loads of times. Here's an example (I've cut down complexity just to show with in action):

        ContainerTag tbody = tbody();
        List<String> lines = classification.getLines();
        int lineIndex = 0;
        for (String line : lines) {
            ContainerTag tr = tr();
            tr = tr.with(td((lineIndex + 1) + ". " + line));
            tbody = tbody.with(tr);
            lineIndex = lineIndex + 1;
        }
        table = table.with(tbody);
        div = div.with(table);
        return div.render();

Please don't change the behaviour of the with methods, they are so useful as they are. Your lib is so very well suited to build up complex html structures, and to have the with-ers append by default makes for really easy to understand code.

[tipsy]

Please don't change the behaviour of the with methods, they are so useful as they are.

I won't change it anytime soon. I would need to run a survey first, and depending on the results, make the change for version 2 of the library.

Your code would be a lot easier to read without using with to append, IMO. It would look just like HTML:

List<String> lines = Arrays.asList("Line 1", "Line 2", "Line 3", "Line 4");

div(
    table(
        tbody(
            each(lines, line -> tr(
                td((lines.indexOf(line) + 1) + ". " + line)
            ))
        )
    )
).render();

// vs

ContainerTag div = div();
ContainerTag table = table();
ContainerTag tbody = tbody();
int lineIndex = 0;
for (String line : lines) {
    ContainerTag tr = tr();
    tr = tr.with(td((lineIndex + 1) + ". " + line));
    tbody = tbody.with(tr);
    lineIndex = lineIndex + 1;
}
table = table.with(tbody);
div = div.with(table);
return div.render();

I'll stop trying to convert you though. I've made a note of the inconsistency, and I'll think about what to do.

[h2software]

No problem. Thank you.

[kicktipp]

I think we should keep the behavior and even extend withClass or we add some new methods addClass() and add(). Everybody should use the lib as he or she wants. I used mulitple with(), too and didn't even noticed it until I came across this discussion.

[ijabz]

Just to note

1. I also tripped over on this issue, I made call to withClass() class calls and it took me some time to realize that one had overwritten the another.
2. I was using the declarative way and was totally unaware of the with() method. However I found it could get a bit unwieldly and difficult to put everything within a html() call, so I have started to use with() more and this has made my code clearer.
   Also because of the problem that each() calls always render html as a single line making it hard to debug Im finding myself using a standard for loop and then adding to its parent using with().

[QuintinWillison]

I also hit this. For what it's worth, my assumption was definitely that withClass(String) would accumulate (append) rather than reset (replace). That assumption was based on my knowledge of the HTML attribute being targeted, always documented as 'classes', along with commonly accepted English use of the word "with".

[tipsy]

@QuintinWillison do you think withX's behavior should depend on the targeted attribute?

[QuintinWillison]

@tipsy yes, indeed. Though my analysis is likely flippant - you clearly have a much deeper understanding of the wider design picture. I was just commenting based on my context and setting - which, as we all do, carries lots of assumptions with it. Thanks.

[ijabz]

Forgive my naivety but I dont understand when you would ever want with() to replace rather than append. Since j2html is always constructing html to eventually be rendered in a html page wouldnt multiple calls to with() on an element that just mean you were overwriting an earlier call you made and therefore why make that first call in the first place.

[tipsy]

Forgive my naivety but I dont understand when you would ever want with() to replace rather than append. Since j2html is always constructing html to eventually be rendered in a html page wouldnt multiple calls to with() on an element that just mean you were overwriting an earlier call you made and therefore why make that first call in the first place.

The idea behind j2html was to allow people to write declarative Java code with a 1-1 HTML mapping. I didn't anticipate people using it with mutable/temporary objects.

As I said in a comment a little up, intended use is:

tbody(
    each(lines, line -> tr(
        td((lines.indexOf(line) + 1) + ". " + line)
    ))
)

Not intended use:

ContainerTag tbody = tbody();
int lineIndex = 0;
for (String line : lines) {
    ContainerTag tr = tr();
    tr = tr.with(td((lineIndex + 1) + ". " + line));
    tbody = tbody.with(tr);
    lineIndex = lineIndex + 1;
}

If you look at the examples on the website, all the code-snippets follows this declarative style where you never save a reference, and you never alter an object after its creation.

I'm not saying people are wrong for not following this style, but that's why methods don't append (ContainerTag#with's current behavior is a bug).

[tipsy]

@ijabz Also because of the problem that each() calls always render html as a single line making it hard to debug Im finding myself using a standard for loop and then adding to its parent using with().

You can use .renderFormatted() for debugging.

[ijabz]

Until 1.3 I couldn't because of #97 but yes hopefully should now work

[mdewilde]

Also hit this.
I would propose adding a method addClass to append a class. This is well known to all who ever worked with jQuery, for example, and would allow retaining existing j2html idiom.

[kevinkrouse]

We also hit this.

[lambdaupb]

Hit this as well, this is very surprising behaviour.

Especially since ContainerTag.with(...) does add and not replace.

A lot of times elements have a base set of classes and need some conditional classes added based on validation etc.

like

input(...)
    .withClasses("form-control", "light")
    .withCondClass(!validator.validate(field), "input-invalid")

Sure its possible to stuff tenary expressions into the .withClasses(..) arg list, but that is really ugly.

While changing the behaviour of .withClass* from replace to add might be debatable, an ergonomic way to fluently add a class can be added without a breaking change.

[sembler]

@lambdaupb your example is one that convinces me that appending behavior is needed. I've favored the iff() approach for the most part, but having the method calls laid out like that would be much clearer to anyone reading the code. The difficulty of how to make this behavior clear when compared to other attribute-adding methods is what I've been thinking over for the past week. While I don't have firm answers yet, we'll definitely have this implemented in version 2.