Skip to:
Content

BuddyPress.org

Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#5647 closed defect (bug) (fixed)

xProfileFields - deletion of drop down select box values not possible

Reported by: GreenFeetz Owned by: djpaul
Milestone: 2.0.2 Priority: high
Severity: normal Version: 2.0
Component: Extended Profile Keywords: needs-patch
Cc:

Description

Hi all,

not sure if this is truly a defect, or me just being stupid, but currently I cannot delete values from drop down select box anymore. The [x] behind the value is missing for existing values.

WP: 3.9.1
BP: 2.0.1

Attachments (2)

Unbenannt.JPG (32.3 KB) - added by GreenFeetz 6 years ago.
5647.patch (1.1 KB) - added by imath 6 years ago.

Download all attachments as: .zip

Change History (12)

@GreenFeetz
6 years ago

#1 @imath
6 years ago

  • Keywords has-patch 2nd-opinion added

Hi GreenFeetz

Thanks for your feedback. I confirm it's a regression from 1.9.2. Just tested, as you said in previous versions it was possible to delete an option while editing a profile field. It's no more the case.

i'm suggesting 5647.patch to solve the issue. i think this should be in 2.0 branch.

@imath
6 years ago

#2 follow-up: @DJPaul
6 years ago

Some feedback on the patch.

  • -1 appears to be a magic number. What is -1? Why don't we want to render this bit of the template for -1? It also appears to be not in the 1.9.2 version; did I change the logic in the 2.0 version that made this extra check necessary?
  • For id="delete-<?php echo esc_attr( $options[$i]->id ); ?>", it's neater to concatenate delete- with the variable inside the esc_attr() call (similar how to you built the href value), e.g. id="<?php echo esc_attr( 'delete-' . $options[$i]->id ); ?>"

While [x] is a lousy, untranslatable way of indicating a delete button (which is the fault of an old version of BP), I don't think we need to adjust that for this ticket, but we should make another ticket to go through and replace ticks/crosses with genericons or dashicons or something similar.

#3 in reply to: ↑ 2 @imath
6 years ago

Replying to DJPaul:

Some feedback on the patch.

  • -1 appears to be a magic number. What is -1? Why don't we want to render this bit of the template for -1? It also appears to be not in the 1.9.2 version; did I change the logic in the 2.0 version that made this extra check necessary?

I think you did change the logic see line 2811 of bp-xprofile-classes.php. In the comment above you're indicating :

// If there are still no children options set, this must be the "new field" screen, so add one new/empty option.

So i thought, maybe wrongly, that -1 means it's a new field and we shouldn't delete an option that doesnot exist, like it was the case in 1.9.2 for instance. We could alternatively check if the $options[$i]->name is not empty.

  • For id="delete-<?php echo esc_attr( $options[$i]->id ); ?>", it's neater to concatenate delete- with the variable inside the esc_attr() call (similar how to you built the href value), e.g. id="<?php echo esc_attr( 'delete-' . $options[$i]->id ); ?>"

Yeah, i've just copied the 1.9.2 line, sorry for the shortcut. I'll work on a new patch depending on your preference about point 1

While [x] is a lousy, untranslatable way of indicating a delete button (which is the fault of an old version of BP), I don't think we need to adjust that for this ticket, but we should make another ticket to go through and replace ticks/crosses with genericons or dashicons or something similar.

I agree the trash dashicon for instance.

Last edited 6 years ago by imath (previous) (diff)

#4 @imath
6 years ago

  • Milestone changed from Awaiting Review to 2.0.2

As it's a regression from 1.9, i think we should try to fix this for 2.0.2

#5 @DJPaul
6 years ago

  • Keywords needs-patch added; has-patch 2nd-opinion removed
  • Milestone changed from 2.0.2 to 2.1
  • Priority changed from normal to high

#6 @DJPaul
6 years ago

  • Milestone changed from 2.1 to 2.0.2

Didn't mean to move this

#7 @DJPaul
6 years ago

imath; looks like JJJ accidentally fixed this in r8643

Can you check and consider how best to fix this for the branch?

#8 @djpaul
6 years ago

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

In 8657:

xProfile: re-instate profile field option delete button.

It was accidentally removed in the 2.0 release during the xProfile Field Types re-implementation.
Fixes #5647, props imath

#9 @DJPaul
6 years ago

Decided to fix by reinstating code from the 1.9 branch, via imath's patch. The -1 conditional wasn't required, and this now works the same in the 2.0 branch and 1.9 branch, according to my testing.

#10 @imath
5 years ago

In 8739:

In case of an xProfile field requiring options (eg: selectbox, radio buttons), make sure the first option doesn't show a delete link when creating or editing this type of fields.

See #5647

Note: See TracTickets for help on using tickets.