WW-4634 Centre alignment doesn't seem to work in Velocity tags

[victorsosa]

Fix for WW-4634

Centre alignment doesn't seem to work in Velocity tags

[lukaszlenart]

Can you use - to separate prefix and suffix?

[victorsosa]

You mean like this class="tdAlign-${parameters.align?html}" ?

[lukaszlenart]

yes

[victorsosa]

Done

[lukaszlenart]

This doesn't fix the main issue, right?

[victorsosa]

Yes, it does fix the main issue and keep the back compatibility.

[victorsosa]

Another thing I will change the name to just

align-center

because this css class can be used in other places that are using the align attribute; which is s Not supported in HTML5.

[aleksandr-m]

Why not just add some CSS class to this table cell? Then end-users can style it as they want and align parameter will not be needed.

[victorsosa]

CSS class to this table cell is what we have now and to have back compatibility as @lukaszlenart said.

[lukaszlenart]

I'm just wondering what was the main reason, I have prepared a unit test and it didn't pass when using 2.3.29-SNAPSHOT which means 2.3.28.1 is broken the same way.

[victorsosa]

@lukaszlenart the parameter align was remove on this commit
victorsosa@a0b3480

[aleksandr-m]

Issue is about Velocity tags, not FreeMarker. Are we still supporting Velocity tags?

[lukaszlenart]

So it's not a regression bug ;-) Yes, we do support Velocity.

[aleksandr-m]

How the Velocity templating is working? The only Velocity templates I see are in the archive.

[aleksandr-m]

Yep, the aforementioned commit misses css class for that cell in the controlheader.ftl. It should be the same way as for the buttons. :)

[victorsosa]

OH ok, got you https://github.com/apache/struts/tree/master/core/src/main/resources/template/archive I missed that.

[victorsosa]

the css class for that cell is in the controlheader.ftl already.

[lukaszlenart]

All the tags are implemented in FreeMarker, those archive templates are deprecated and should be thrown away.

[aleksandr-m]

@lukaszlenart Ahh ok. Thanks for clarifying that.

[aleksandr-m]

@victorsosa I mean something like <td class="tdInput"> in controlheader.ftl and .tdInput {text-align:left;} in styles.css. <-- This is for 2.5.

And if alignment doesn't work in 2.3 and your solution fixes it, then this PR should target support-2-3 branch.

[victorsosa]

It will break back compatibility as @lukaszlenart said.

[aleksandr-m]

@victorsosa You see, this commit already broke backward compatibility, but it was incomplete in the way it didn't introduce css class for the input cell.
It is 2.5 so some breaking changes are expected.

[lukaszlenart]

@victorsosa this commit a0b3480 dropped align attribute for buttons, it shouldn't affect textfield tag. Anyway, I think we should merge this PR and if needed prepare another one to target 2.3

[victorsosa]

@lukaszlenart ok

[aleksandr-m]

I would prefer to follow html5 and drop align attribute completely from S2 tags, BUT @victorsosa can you at least add some css class to input cell along with your solution.
Something like:

if align then class="tdInput align-${parameters.align?html}" else class="tdInput"

[victorsosa]

@aleksandr-m You mean something like <td class="tdInput"> in controlheader.ftl and .tdInput {text-align:left;} in styles.css. <-- This is for 2.5.

THat means drop align attribute completely from S2 tags; So from now on the only way to set the align to center for example is to overwrite the css class tdInput in 2.5, right?

I will change that code.

[aleksandr-m]

@victorsosa IMO it would be great. BUT if we really really want it to be backward compatible then we can do both. Add a class for future and use align attribute with class="align-${parameters.align?html}" like you propose in this PR.

[cnenning]

👍

[lukaszlenart]

Tests are failing :(

Error Message

expected:<...bel:</label></td><td[]><labelid="myname"ti...> but was:<...bel:</label></td><td[class="tdInput"]><labelid="myname"ti...>
Stacktrace

junit.framework.ComparisonFailure: expected:<...bel:</label></td><td[]><labelid="myname"ti...> but was:<...bel:</label></td><td[class="tdInput"]><labelid="myname"ti...>
    at junit.framework.Assert.assertEquals(Assert.java:100)
    at junit.framework.Assert.assertEquals(Assert.java:107)
    at junit.framework.TestCase.assertEquals(TestCase.java:269)
    at org.apache.struts2.views.jsp.AbstractUITagTest.verify(AbstractUITagTest.java:246)
    at org.apache.struts2.views.jsp.ui.LabelTest.testSimple(LabelTest.java:49)

[lukaszlenart]

103 tests need to be updated, I think it isn't worth

[victorsosa]

I will fix the 103 test cases, give me some time

[lukaszlenart]

@victorsosa osm!

[lukaszlenart]

I assume we're ok with this and this PR can be merged?

[victorsosa]

Yes, I am ok

[aleksandr-m]

Looks fine. Good job.