http://codereview.appspot.com/217084/diff/1/2
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/CajaContentRewriter.java
(right):
http://codereview.appspot.com/217084/diff/1/2#newcode157
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/CajaContentRewriter.java:157:
messagesNode.setAttribute("style", "display: none");
On 2010/02/24 19:21:26, MikeSamuel wrote:
Can this line move to the formatErrors, or can the node creation move
here?
Setting an attribute clobbers anything that was there before, so it
seems best
to have the stuff that sets up the DOM envelope in one place so that
there's
less chance of someone setting a style there, and it being clobbered
here.
Done.
http://codereview.appspot.com/217084/diff/1/2#newcode201
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/CajaContentRewriter.java:201:
errElement.setAttribute("class", "gadgets-messages");
On 2010/02/24 19:21:26, MikeSamuel wrote:
Maybe a comment as to where one would look for styles associated with
this
class.
Done.
http://codereview.appspot.com/217084/diff/1/2#newcode210
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/CajaContentRewriter.java:210:
.append(snippet);
On 2010/02/24 19:21:26, MikeSamuel wrote:
Is there a reason the messageText is created outside this if, and a
reason why
you don't do
msg.getMessageLevel().name() + " " + html(...) + snippet
?
Do you need a break of some kind between the formatted message and the
snippet?
Should the HtmlSnippetProducer be used at 198 instead? It does have
some
vestigial things, such as generating a call to an undefined function
to scroll
to the source. So maybe not.
Done.
I am not using HtmlSnippetProducer for exactly the reason you mention.
http://codereview.appspot.com/217084/diff/1/3
File javascript/container/gadgets.css (right):
http://codereview.appspot.com/217084/diff/1/3#newcode63
javascript/container/gadgets.css:63: .gadgets-messages {
On 2010/02/24 19:21:26, MikeSamuel wrote:
Maybe add a comment as to where this is generated.
Done.
http://codereview.appspot.com/217084/show
