Skip to:
Content

BuddyPress.org

Opened 13 years ago

Closed 13 years ago

Last modified 8 years ago

#3074 closed defect (bug) (fixed)

XProfile - Incorrect CSS zebra striping

Reported by: ezd's profile ezd Owned by:
Milestone: 1.5 Priority: normal
Severity: normal Version: 1.5
Component: Templates Keywords: has-patch
Cc:

Description

In my profile where I can enter different information about myself (About Me, Work ect) every second line should be a bit shaded/darker.

Right now every row in my "Work" section is shaded: http://testbp.org/members/ezcali/profile/

(Tested on testbp.org)

Attachments (4)

3074.01.patch (1.8 KB) - added by r-a-y 13 years ago.
3074.02.patch (1.4 KB) - added by r-a-y 13 years ago.
3074.03.patch (5.5 KB) - added by boonebgorges 13 years ago.
3074.04.patch (7.1 KB) - added by johnjamesjacoby 13 years ago.
Attached patch fixes issue with querying empty fields and zebra striping. Also removes a fix on the Profile view for an old issue where the Base XProfile Group couldn't be renamed, where we hid the "Base" title.

Download all attachments as: .zip

Change History (21)

@r-a-y
13 years ago

#1 @r-a-y
13 years ago

  • Component changed from Core to XProfile
  • Keywords has-patch added
  • Summary changed from Profile information to XProfile - Incorrect CSS zebra striping

The problem is a conflict between bp_get_field_css_class() and the JS which zebra-stripes the tables.

The solution is to get rid of the manual "alt" class in bp_get_field_css_class().

Attached patch does this and also trims some trailing whitespace.

@r-a-y
13 years ago

#2 @r-a-y
13 years ago

  • Component changed from XProfile to Theme

The first patch highlights a problem with jQuery's odd selector:
http://trevordavis.net/blog/jquery-table-striping-bug/

Example:
http://img263.imageshack.us/img263/1017/zebrac.png

BP does it right the first time ;)

Second patch proposes changing jQuery's :odd selector to :nth-child(even) to properly highlight alternate rows.

Version 0, edited 13 years ago by r-a-y (next)

#4 @DJPaul
13 years ago

  • Milestone changed from Awaiting Review to 1.3

Really, why are we zebra striping with javascript? :nth-child(2n+1)

#5 @r-a-y
13 years ago

Don't ask me why! I'm just detailing the bug and workarounds.

#6 @djpaul
13 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [4287]) Correct zebra striping. Fixes #3074

#7 @hnla
13 years ago

I may be being extremely thick here and have only glanced over the changeset 4287 quickly when I saw it pop up on twitter Paul but it reads as though you have deleted ALL the functionality for zebra stripping i.e all the JS setting :nth child 'alt' classes on a table with class 'zebra' has been deleted??!!

#8 @DJPaul
13 years ago

Where we want zebra striping, we've manually put 'alt' as a class for the appropriate rows via the _css_class() helpers. Didn't need the javascript, nor the old 'zebra' class, so I took it out.

#9 @hnla
13 years ago

Ah ok that perhaps wasn't clear, yes it makes sense to add it via serverSide script rather than clientSide as it's a preferable approach.

#10 @boonebgorges
13 years ago

  • Resolution fixed deleted
  • Severity set to normal
  • Status changed from closed to reopened

Reopening due to the fact that optional but un-filled-in profile fields are still throwing the zebra count off.

3074.03.patch adds a 'hide_empty_fields' parameter to the bp_has_profile() chain. When the parameter is set to true (which is the default when you're not on the Dashboard or looking at a user profile edit page), fields that do not have data corresponding to them are removed from the $profile_template object that is returned to the templating functions.

@johnjamesjacoby
13 years ago

Attached patch fixes issue with querying empty fields and zebra striping. Also removes a fix on the Profile view for an old issue where the Base XProfile Group couldn't be renamed, where we hid the "Base" title.

#11 follow-up: @boonebgorges
13 years ago

Re 3074.04.patch. Your IS NOT NULL query assumes that blank rows exist in the profile data table for optional fields left blank. This may be the case during a normal profile edit (I would have to check) but it will not be the case if the admin creates a new profile field after the site is up and running. I'll grant that this is an edge case, and that your method is a bit easier to follow, but the method in 3074.03.patch does not have this shortcoming.

Also, as DJPaul pointed out in a private channel, the static $alt_row; is unnecessary since the query won't be returning empty rows. We can use a modulo instead.

Last edited 13 years ago by boonebgorges (previous) (diff)

#12 in reply to: ↑ 11 @johnjamesjacoby
13 years ago

Replying to boonebgorges:

Re 3074.04.patch. Your IS NOT NULL query assumes that blank rows exist in the profile data table for optional fields left blank. This may be the case during a normal profile edit (I would have to check) but it will not be the case if the admin creates a new profile field after the site is up and running. I'll grant that this is an edge case, and that your method is a bit easier to follow, but the method in 3074.03.patch does not have this shortcoming.

Also, as DJPaul pointed out in a private channel, the static $alt_row; is unnecessary since the query won't be returning empty rows. We can use a modulo instead.

I'm not sure I understand what you mean. When you edit a profile, you're not using $hide_empty_fields so that won't matter. The 'IS NOT NULL' part of 3074.04.patch assumes we don't want to return empty or null values, which IS what we want when viewing a profile. Even if a field is required, the user has no data entered so there is nothing to show.

Either we want empty fields or we don't, required or not?

$alt_row is necessary because the modulo is based on if the field_id is an even or odd number, which when skipping empties or having them be rearranged, the ID's could be anything. This is why I went the JS route originally - it was an easy way to consistently handle this all until we look at it later, which is now. :)

#13 @boonebgorges
13 years ago

Two things:

  • The way xprofile data is stored, the following queries are (I believe) the same:

SELECT field_id, value FROM $bp->profile->table_name_data WHERE field_id IN ( { $field_ids_sql } ) AND user_id = %d;
SELECT field_id, value FROM $bp->profile->table_name_data WHERE field_id IN ( { $field_ids_sql } ) AND user_id = %d AND field_id != '' AND field_id IS NOT NULL;

That's because xprofile does not save blank rows. So, say there is an optional profile field called "School", with field_id = 5. When I save my profile, if I do not provide a value for School, xprofile skips it. It does *not* save a row that looks like this:
+----+----------+---------+------------------------+---------------------+
| id | field_id | user_id | value | last_updated |
+----+----------+---------+------------------------+---------------------+
| 10 | 5 | 1 | | 2011-07-26 16:10:41 |

That's what I mean by "no blank rows are saved". So your $hide_empty_fields clause doesn't do anything, because there will never be rows with a blank value.

  • The second issue is that we want to hide empty FIELDS, not empty FIELD DATA. $fields is populated by a *separate query*, several lines before $field_data is populated. If we want to hide empty fields, then we either need to turn this into a single query with a directional join (so that fields are skipped when there is no corresponding data in the data field), or we need to loop through $fields *after* querying for $field_data, and unset individual fields based on $field_data. That's what 3074.03.patch does.

This is a bit complicated, hope I've explained it properly :) It might be easier if you just do a var_dump of $groups just before it's returned by BP_Xprofile_Group::get() - you'll see that your method leaves dataless fields, even with your additional query clause.

#14 @johnjamesjacoby
13 years ago

Hm. That makes sense. A combination of our patches is the best solution then?

#15 @boonebgorges
13 years ago

I was just glancing over the code, and it looks like the $profile_template->current_field property is an iterator, and does *not* match the field ID. That means that we can use a modulo on that value for zebras.

I'll work on a combined patch.

#16 @boonebgorges
13 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [4869]) Adds hide_empty_fields parameter to bp_has_profile() and the rest of the profile template chain, to make zebra striping easier in the profile component. Fixes #3074. Props johnjamesjacoby for help with the patch

#17 @DJPaul
8 years ago

  • Component changed from Appearance - Template Parts to Templates
Note: See TracTickets for help on using tickets.