launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Review: Needs Fixing code

This markup is incompatible with the expander. The expander *creates* an <a> node and wraps the label node to create the expanding link. The label node is the first node in the "collapsible" parent, and it is already an anchor. Anchors in anchors are invalid and fail in some browsers. You need to change the first node to a span., of but that markup also looks odd because we have a link in the span...

We are being stupid. This markup has been edited so many times over seven years we forgot what is happening.
1. We do not need a link to the form page, the bloody form is rendered inline.
2. The form should not be hidden, the expander adds the hidden css class when js is successful, and we care about hide-on-load
3. We already have a driver permission check in the outer nodes, we don't need checks in the subnodes.
4. the "lesser" css class that makes the small text in a for we expect to use is madness. the small text is for asides you do not need to do important work with. We can remove that.
5. I think we want to tweak the style, or find a class that does the proper alignment and spacing. The current alignment was probably designed for Lp 1.0.
6. Oh the secondary class is not used with tables anymore

This is a replacement template that removes the cruft. http://pastebin.ubuntu.com/1260024/
I would be nice if the form knew it was embedded, but that is really a seperate issue involving formoverlay.
-- 
https://code.launchpad.net/~wallyworld/launchpad/nominations-javascript2-271697/+merge/127918
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.