This pretty much already worked, but if multiple filter_tables containing the same type of objects existed on a page, they would end up with the same ID, which is illegal in HTML. I also fix a silly bug where a dropdown had the name "select_pools" when "pools" should not have been hard-coded.
-- Matt
Previously, they could share an id, which is illegal HTML. Also fixes an unrelated bug with a hardcoded table name. --- src/app/views/layouts/_filter_table.haml | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
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) = object_count.to_s + " results" = text_field_tag :q, search_term || "Search result for...", :disabled => true = link_to('Filters', '#') %p = link_to('Share this view', '#') = link_to('Add/Remove Columns', '#') - %table(id="#{lower_class}_table") + %table(class="#{lower_class}_table") %thead %tr - columns.each do |column|
On 06/10/2011 09:06 PM, Matt Wagner wrote:
Previously, they could share an id, which is illegal HTML. Also fixes an unrelated bug with a hardcoded table name.
src/app/views/layouts/_filter_table.haml | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
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.
= object_count.to_s + " results" = text_field_tag :q, search_term || "Search result for...", :disabled => true = link_to('Filters', '#') %p = link_to('Share this view', '#') = link_to('Add/Remove Columns', '#')
- %table(id="#{lower_class}_table")
- %table(class="#{lower_class}_table") %thead %tr - columns.each do |column|
Hey Matt,
Conditional ACK, see inline.
Thomas
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.
-- Matt
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
On 06/14/2011 11:21 PM, Matt Wagner wrote:
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
Yeah, I'm fine with that, just wanted to make sure we're aware of the limitation. Feel free to push the code, Matt.
Thomas
On Wed, Jun 15, 2011 at 10:30:23AM +0200, Tomas Sedovic wrote:
Yeah, I'm fine with that, just wanted to make sure we're aware of the limitation. Feel free to push the code, Matt.
Pushed. Thanks for pointing this out and getting us thinking about this.
-- Matt
aeolus-devel@lists.fedorahosted.org