[buddypress-trac] [BuddyPress Trac] #5220: Overhaul implementation of xprofile field types

[buddypress-trac]

#5220: Overhaul implementation of xprofile field types
------------------------------------------+-----------------------
 Reporter:  DJPaul                        |       Owner:  DJPaul
     Type:  enhancement                   |      Status:  assigned
 Priority:  normal                        |   Milestone:  2.0
Component:  XProfile                      |     Version:
 Severity:  normal                        |  Resolution:
 Keywords:  has-patch dev-feedback early  |
------------------------------------------+-----------------------

Comment (by DJPaul):

 Thanks for taking a detailed look at the patch; I appreciate it. I left a
 comment on your github pull request as it seems there was a small mistake
 in using _ instead of an _e function in a few lines, but all the other
 suggestions look fine. I'll keep an eye on github and merge the pull
 request in once resolved.


 > The markup for the checkbox and radiobutton types doesn't seem to be
 quite right.
 > The "(required)" text seems to be repeated almost verbatim in almost
 every instance of edit_field_html().
 I don't disagree that these stand out as pain points, but so far in this
 patch, I've taken the approach of being very conservative about not
 changing code that doesn't strictly need to be changed for the purpose of
 implementing the new field type classes. Mainly I didn't want to add all
 this new stuff and change some of the existing templating markup in one
 go. I think I would prefer dealing with these in separate tickets so we
 can get more opinions on those specific parts.


 > The "(required)" text seems to be repeated almost verbatim in almost
 every instance of edit_field_html().
 For this specifically, if it's abstractable, sure. I think however the
 real problem is that this probably sucks as a translatable string (no
 comments or context, plus it's a bit weird), and improving that would be a
 higher priority.


 > There's an [x] to remove the first option on a supports_option field,
 but the js attached to it is broken because of some odd selectors
 I'll check this out and get it fixed; thanks.


 > I see in is_valid() that you've chosen not to validate if no rules are
 matched, even when no rules have been set...
 I don't have a strong opinion which way we do this. I *would* like
 developers to make decisions about the formats that their custom field
 types accept.


 > Two, /.+$/ will fail for empty strings... is this an unnecessary
 restriction?
 I can't see the issue. I've just added more tests. What type of profile
 field are you thinking of? e.g. For textboxes, it uses /.*/ instead.

--
Ticket URL: <https://buddypress.trac.wordpress.org/ticket/5220#comment:16>
BuddyPress Trac <http://buddypress.org/>
BuddyPress Trac