-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
Use HTML templating in default UIs #15580
Conversation
173a728
to
41411c5
Compare
87ba3d0
to
7a2e673
Compare
9363b22
to
1c4bc7a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. I've left a few comments inline.
I also wonder if for the tests (that we have updated) if we can reduce the use of +
and \n
on the large Strings? I can understand the desire to avoid using the templates in the tests, but perhaps """
can still be used with minimal +
of the variables. Alternatively, if we know the value of the variables we could just use a large constant there and assert that the value of the variables matches our constant.
margin: 0; | ||
line-height: 1.5; | ||
} | ||
\s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious if there is a need for the \s
escape sequence in the Strings? Is this related to the checkstyle problems?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost, the problem is the formatter, not checkstyle. Spring-javaformat deletes trailing spaces on lines, but not \s
. Checkstyle does not complain.
Our text blocks do contain some blank lines with spaces, mostly due to calling .indent(...)
on the CSS style blocks. So in our tests, we need to preserve the blank, non-empty lines in text-blocks. The vast majority will go away once we move the CSS out into a "static" resource.
web/src/main/java/org/springframework/security/web/server/ui/HtmlTemplates.java
Outdated
Show resolved
Hide resolved
0f060e5
to
3875dd1
Compare
Addressed, using text blocks in tests too. |
3875dd1
to
0114a4e
Compare
0114a4e
to
4f6ca21
Compare
4f6ca21
to
7b5d983
Compare
7b5d983
to
194ccbc
Compare
reviewer: @rwinch
Some notes:
I have not updated the tests, they still use string concatenation sometimes. I'd rather not use the templating in the tests themselves.Using text blocks and constants in tests.For context, checktyle / SpringLeadingWhitespace breaks when using space-indented text-blocks. There is an open issue in spring-javaformat ;This is is Fixed in spring-javaformat 0.0.43//@formatter:off
does not suppress the error. I have re-indented all text blocks with tabs. If we really prefer spaces, then we would need file-wide checkstyle suppressions.