Skip to:
Content

Opened 8 years ago

Closed 8 years ago

Last modified 3 years ago

#3074 closed defect (bug) (fixed)

XProfile - Incorrect CSS zebra striping

Reported by: 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 8 years ago.
3074.02.patch (1.4 KB) - added by r-a-y 8 years ago.
3074.03.patch (5.5 KB) - added by boonebgorges 8 years ago.
3074.04.patch (7.1 KB) - added by johnjamesjacoby 8 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
8 years ago

#1 @r-a-y
8 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
8 years ago

#2 @r-a-y
8 years ago

  • Component changed from XProfile to Theme

The first patch highlights a problem with using jQuery's odd selector to zebra-stripe:
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.

Last edited 8 years ago by r-a-y (previous) (diff)

#4 @DJPaul
8 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
8 years ago

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

#6 @djpaul
8 years ago

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

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

#7 @hnla
8 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
8 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
8 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
8 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
8 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
8 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 8 years ago by boonebgorges (previous) (diff)

#12 in reply to: ↑ 11 @johnjamesjacoby
8 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
8 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
8 years ago

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

#15 @boonebgorges
8 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
8 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
3 years ago

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