On Mon, Jun 13, 2011 at 09:47:04AM -0400, Matt Wagner wrote:
On Mon, Jun 13, 2011 at 01:09:42PM +0200, Tomas Sedovic wrote:
diff --git a/src/app/views/layouts/_filter_table.haml b/src/app/views/layouts/_filter_table.haml index 5fd716d..1c50d2d 100644 --- a/src/app/views/layouts/_filter_table.haml +++ b/src/app/views/layouts/_filter_table.haml @@ -3,14 +3,14 @@ = form_header %p Viewing
- = select_tag("select_pools", saved_searches.join.html_safe,:disabled => true)
- = select_tag("select_#{lower_class}", saved_searches.join.html_safe,:disabled => true)
The `select_tag` helper sets the `id` and the `name` attribute (both of which have to be unique).
That means we can't have, say, two tables listing Pools on the same page.
Do you think that's a reasonable assumption to make? If so, I'm willing to let this in, but I think it would be better to slap some randomness at the end or possibly move the text into a class as well.
Oh, nice catch. While it's unlikely that we'll have multiple instances of the same object type on the page, the other half of this patch was intended to permit exactly that use case, so this is pretty much a fail on my part.
I'll have to find a better way to implement this and re-send.
Okay, so I just revisited this and have changed my mind. After speaking a bit with Ken, it seems that it's (a) unlikely that we will ever have two filter_tables for objects of the same class on the same page, and (b) likely that we're going to change the code around filtering anyway.
I thought about randomizing the name and tried appending rows.object_id to the end, but the problem is that this requires whatever code receives it to do something nutty like iterate through params, do a substring match for what it's expecting, and strip the numbers off the end.
So I think it makes the most sense to apply this patch as-is, understanding that we don't expect to have more than one table in a class on the page at the same time, and that if that assumption changes, we're going to want to find a more elegant way to differentiate them anyway.
Does this make sense?
-- Matt