Skip to:
Content

BuddyPress.org

Opened 4 years ago

Closed 4 years ago

#6695 closed defect (bug) (fixed)

xProfile Draggable Admin UI needs the fieldsets to have ids

Reported by: imath Owned by: imath
Milestone: 2.4 Priority: normal
Severity: blocker Version:
Component: Extended Profile Keywords: has-patch needs-testing commit
Cc:

Description

As @dimensionmedia reported on slack, it appears r10071 introduced a regression into the xProfile Draggable Admin UI :
-> Ajax requests to set the order of fields or moving a field from a group of field to another failed because the fieldset#ids were missing.

I suggest to keep the improvement of avoiding having duplicate ids on this page by prefixing the value of ids with a draggable_ prefix and by editing the Ajax action so that it keeps on working the right way.

The attached patch is my suggestion.

Attachments (1)

6695.patch (1019 bytes) - added by imath 4 years ago.

Download all attachments as: .zip

Change History (8)

@imath
4 years ago

#1 @DJPaul
4 years ago

  • Keywords needs-testing added

Looks okay, but needs to be tested by at least a couple of people, otherwise we should revert r10071 and ship the change in a future release.

#2 @DJPaul
4 years ago

  • Severity changed from normal to blocker

#3 @mercime
4 years ago

Thank you for the patch @imath :) Tested patch on the following installations:

  • WP 4.3.1 and BP 2.3.4 upgraded to BP 2.4 RC1 - Works
  • WP 4.4 trunk and BP 2.4 trunk - Works
  • Multisite, BP is Network-Activated - WP 4.4 trunk and BP 2.4 trunk - Works.

#4 @hnla
4 years ago

Tested patch on:
WP 4.4 beta-1 & BP 2.4.0 rc1

Behaviour is back to expected. I can move fields back and fourth between groups repeatedly without issue, field drops onto selected group tab and bring focus to group smoothly.

Tested frontend and changes are correctly reflected in displayed groups, editing both frontend and backend values before and after dragging and all seems stable.

@imath one question with the fieldset ids is there a reason we are undescoring between words rather than hyphenating?

Token naming is good though it's clear that the ID's have a specific purpose, if not specific like these I would like all tokens that do, say, hook to scripting to have at least '-js' appended or prepended to names.

Last edited 4 years ago by hnla (previous) (diff)

#5 @DJPaul
4 years ago

  • Keywords commit added

Great feedback, yes, CSS should use a hypen (-). Thanks for testing @hnla.

@imath let's get this in

#6 @imath
4 years ago

Thank you all for your great feedbacks :) I will commit it in a few hours.

#7 @imath
4 years ago

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

In 10324:

xProfile Admin UI: make sure profile fields can be reordored and moved from a group of fields to another

In r10071 the fieldset ids has been removed to avoid having duplicate ids into the xProfile admin screen. Unfortunately, to save the state of the draggable fields, we need every xProfile fieldsets to have an id attribute. To have the right behavior back, we are putting back an id attribute to these selectors making sure they are unique by prefixing them with "draggable_".

Fixes #6695

Note: See TracTickets for help on using tickets.