search

cfpb / regulations-site

(DEPRECATED) Web interface for viewing U.S. federal regulations and other regulatory information
Other
28 stars 43 forks source link

Notification if inline definition doesn't apply to section #547

Closed theresaanna closed 10 years ago

theresaanna commented 10 years ago

If inline definition that is open doesn't apply to all of section, a warning displays. If it doesn't apply to the active paragraph, a link to refresh the definition displays above grayed out definition text.

ascott1 commented 10 years ago

Overall, I think this is a pretty smart solution to a complex problem.

@cmc333333 do you mind taking a few minutes this morning to review the JS as well.

cmc333333 commented 10 years ago

Python looks fine. The JS that I could follow seemed okay.

Two suggestions:

  1. The checkLinks function uses recursion to touch all the links. Would it make more sense (and be faster) to use straight iteration? I can't imagine links = links.not(links.last()); being fast on sections with many definitions. Good thinking on running this once per definition load/page update (rather than on scroll)
  2. There's markup in the template ("notice hidden group") which you use to display the constructed string. Would it make more sense for the template to contain the string already and you swap out values? As an aside, "notice" might be a bad term here, as notices have a different meaning w.r.t. regulations
theresaanna commented 10 years ago

@cmc333333 1) Yeah, I chose recursion for readability only, really. Using the jQuery object probably does slow it down a bit. I will take another look at the whole thing given Adam's input, too. 2) The reason that I didn't put the string in the template (even though it would be much nicer) is that it shouldn't be visible for people looking at just the output sans CSS or JS. Though, now I realize, those people will never see the inline definition anyway, so yes, I think you're right. Re: "notice", good call. I'll use "warning" or something.