Skip to:
Content

Opened 4 years ago

Closed 3 years ago

#5625 closed enhancement (fixed)

Use wp_editor for "multi line text area" xprofile field in frontend

Reported by: sooskriszta Owned by: boonebgorges
Milestone: 2.4 Priority: normal
Severity: normal Version:
Component: Extended Profile Keywords:
Cc: sooskriszta, needle@…

Description

At the moment, we provide a simple textarea. However, we should be able to offer rich/visual editor (TinyMCE). Use teeny-weeny editor so that there's less noise because of too many buttons.

Should probably also have an option for admin to turn off.

Attachments (6)

5625.patch (9.2 KB) - added by needle 4 years ago.
5625-2.patch (16.4 KB) - added by needle 4 years ago.
5625.03.patch (10.7 KB) - added by boonebgorges 4 years ago.
5625.diff (12.3 KB) - added by boonebgorges 3 years ago.
5625.2.diff (12.7 KB) - added by boonebgorges 3 years ago.
5625.3.diff (13.3 KB) - added by boonebgorges 3 years ago.

Download all attachments as: .zip

Change History (44)

#1 @imath
4 years ago

  • Keywords 2nd-opinion added

hi sooskriszta

Thanks for your feedback, IMO a plugin could take benefit of the great enhancements DJPaul made about the XProfile Type API to create a "rich text" field. I may be wrong, but i'm not sure it absolutely needs to be in core.

#2 @DJPaul
4 years ago

  • Keywords 2nd-opinion removed
  • Milestone changed from Awaiting Review to Future Release

I think there would be no harm to do this, and plugins like bbPress provide an option for both, but I agree it isn't a priority for the core team at the moment, but I'll shift it into our Future Release milestone to give the idea some time to bake, or see if anyone wants to work on a patch.

#4 @DJPaul
4 years ago

  • Keywords dev-feedback added

Cool.

Core team: do we want to make the multi-line text area field type use TinyMCE in 2.2+?

#5 @needle
4 years ago

Related ticket: https://buddypress.trac.wordpress.org/ticket/5742

The above ticket enables allowed tags to be filtered by field type, which is important for a TinyMCE-enabled textarea, since adding images would be one of the things that people are most likely to want to do. If you're interested, switch BP_XPROFILE_RICH_TEXT_FIELD_ADD_MEDIA in the plugin to true to see this in action.

#6 @boonebgorges
4 years ago

  • Keywords needs-patch added; dev-feedback removed
  • Milestone changed from Future Release to 2.2

Core team: do we want to make the multi-line text area field type use TinyMCE in 2.2+?

Yes, I don't see any reason not to. needle, sounds like you may be the best person to write an initial patch for integrating this into BP.

#7 @needle
4 years ago

Happy to do that, Boone.

@needle
4 years ago

#8 follow-up: @needle
4 years ago

  • Keywords has-patch added; needs-patch removed

I've uploaded a preliminary patch for a Visual Editor field type. There are a number of oddities I've addressed via inline comments. I've written the patch so that in theory people who have data already in their systems as a result of using my existing plugin should not lose it if/when the plugin becomes redundant.

Feedback gratefully received.

#9 follow-up: @boonebgorges
4 years ago

  • Keywords 2nd-opinion added

needle - This is a great start. Thanks for your work on it. I'll have some feedback on the smaller oddities in the upcoming weeks, but my initial thought is this: We should not have a separate field type for this. IMHO, all multi-line textarea fields should support rich text. In fact, I think we should just turn it on for those fields and not allow it to be turned off (since there's a Text tab in addition to the Visual tab), though if others feel differently perhaps we could have an admin toggle to disable on a per-field basis. In any case, I don't see a strong case for having a separate field type.

#10 in reply to: ↑ 9 @sooskriszta
4 years ago

Replying to boonebgorges:

admin toggle to disable on a per-field basis.

+1

Replying to boonebgorges:

don't see a strong case for having a separate field type.

+1

#11 in reply to: ↑ 8 @sooskriszta
4 years ago

Replying to needle:

I've uploaded a preliminary patch for a Visual Editor field type.

You are awesome!

@needle
4 years ago

#12 @needle
4 years ago

@boonebgorges Ok, didn't realise. The new patch (5625-2.patch) adds some options to the Textarea field type instead.

There are a few shortcomings with the code in the patch:

  • The logic to decide the defaults for the field type's settings could be more elegant.
  • I'm using the admin_new_field_html() method to display checkboxes for setting various options for the field - is this a misuse of this method?
  • There are scope issues which it would be good to clarify

#13 @boonebgorges
4 years ago

  • Keywords 2nd-opinion removed

needle - Thanks for the revision. 5625.03.patch makes the following changes:

  • Replace your settings check with a function bp_xprofile_is_richtext_enabled_for_field()
  • Tear out the settings stuff. We don't support the display of media at this time, so I can't think of any reason to allow it to be turned on in the UI. I can't think of a reason why we'd want Quicktags ever to be off. And, as I suggested earlier, I can't really come up with a good reason why anyone would want to turn richtext off in general. bp_xprofile_is_richtext_enabled_for_field() has a filter - if people want it disabled for some reason, they can use that. But I'd wager that'll be < 5% of all installations.
  • Remove sanity checks. Given the way these methods are called, I can't see any way that they would ever fail.
  • Codestyling

I'm not 100% clear on the display_filter() and get_field_data() stuff, so I didn't touch that logic for the time being. But I will note that it's pretty hackish indeed, and we've got to find a better solution. There seem to be a couple things going on:

  1. You don't like the auto-linking of xprofile_filter_link_profile_data() on rich text. I'm not sure I follow this one - I messed with it a little and it seemed fine to me.
  2. You want to get raw data in edit mode (add_filter( 'xprofile_get_field_data', array( $this, 'get_field_data' ), 15, 3 );). But don't we already get that? When I comment out this line, everything seems to look OK to me. Can you give the specifics of what you're attempting to prevent?
  3. Certain display filters must be turned off in order for HTML to be displayed properly - in particular, I think, esc_html(). But if that's what we want, then we should just remove_filter(), or change our add_filter() call to a wrapper function that only adds the filter conditionally. Put another way - your patch is approaching this problem like a plugin author (not touching other core files), but a core patch is allowed to touch core files ;)
  4. The technique you're using appears to wreck line breaks.

My gut feeling is that we should tear out your display_filter and get_field_data stuff, and fix what remains of the problems (specifically 3, but maybe also 1) removing the specific filters that are problematic. But before patching this, I wanted to get your feedback, and maybe some more details on the problems you were trying to solve there.

Thanks again! This is going to be a great feature.

This ticket was mentioned in IRC in #buddypress-dev by boonebgorges. View the logs.


4 years ago

#15 @DJPaul
4 years ago

  • Keywords needs-patch added; has-patch removed

@needle Any interesting in helping to get this into 2.2?

#16 @needle
4 years ago

@DJPaul - yes, sorry, I've been snowed under of late. Still have this on my todo list.

#17 @DJPaul
3 years ago

  • Milestone changed from 2.2 to 2.3

Let's try this for 2.3. It's pretty close, and we just need to put some time into it.

#18 @boonebgorges
3 years ago

  • Milestone changed from 2.3 to 2.4

It pains me to do it, but we're going to have to bump this. It's within spitting distance of being done, but someone needs to give it a little push.

#19 @boonebgorges
3 years ago

  • Keywords has-patch added; needs-patch removed
  • Owner set to boonebgorges
  • Status changed from new to assigned

This is so close it hurts. I'm going to make it happen for 2.4.

This ticket was mentioned in Slack in #buddypress by boone. View the logs.


3 years ago

#21 @needle
3 years ago

@boonebgorges - apologies, it just hasn't been possible for me to push this forward. I'll keep an eye on the ticket and help out where I can.

@boonebgorges
3 years ago

#22 @boonebgorges
3 years ago

5625.diff is a non-working attempt to refresh and clean up the existing patch. I'm leaving it aside while I work on #6638, which I believe blocks the current ticket.

#23 @boonebgorges
3 years ago

In 10203:

Pass field ID to xprofile field type filter methods.

display_filter() and pre_validate_filter() are more useful if they receive
the field ID. So we pass it, when available.

See #5625.

@boonebgorges
3 years ago

#24 @boonebgorges
3 years ago

  • Keywords needs-testing added

5625.2.diff is (I think) a working implementation.

I've reworked needle's original patch so that BP_XProfile_Field_Type_Textarea no longer needs to do a bunch of tricks to jump through filter hoops. For one thing, that technique involved adding, removing, and stacking filters in a way that created unnecessary complication. For another, it limited richtext fields to textareas only - a restriction that seems appropriate by default, but should not be enforced strictly.

The most important addition here is bp_xprofile_escape_field_data(). I'm using this instead of esc_html when filtering field data for output. If richtext is enabled for the field, the data is run through kses; if not, it's run through esc_html() like before.

The other changes of note are related to editor CSS. I made a few changes to make it look OK in our default templates. This meant removing the top/bottom padding from the textarea in 'Text' mode (it looked weird when switching between modes), and making buttons a bit narrower so that they squeeze into a single row on most themes. hnla and others, please let me know if this will break anything.

This ticket was mentioned in Slack in #buddypress by boone. View the logs.


3 years ago

#26 @DJPaul
3 years ago

In class-bp-xprofile-field-type-textarea.php line 29, you set type = textarea and check for it later in bp-xprofile-functions.php line 1047 (line numbers taking from viewing the patch file on Trac). Unless I'm misreading this, in the first instance you're setting the type on the BP_XProfile_Field_Type class (such a property isn't declared) and you are reading it from a BP_XProfile_Field (where such a property *is* declared).

Having bp_xprofile_is_richtext_enabled_for_field decide if a field supports rich HTML seems like the wrong place. I think that information should go in the field_type somewhere because it affects how the field is rendered, obviously.

I was going to suggest moving the allowed_kses customisations into the field_type somehow because, again, it's information about a field type, and not a specific instance of a field. Once of the goals and wins behind the introduction of the field_type classes a few releases ago was to centralise all the information about that field_type and avoid special-case logic littered throughout BP, a little like what we are proposing here.

#27 @boonebgorges
3 years ago

Having bp_xprofile_is_richtext_enabled_for_field decide if a field supports rich HTML seems like the wrong place. I think that information should go in the field_type somewhere because it affects how the field is rendered, obviously.

Once of the goals and wins behind the introduction of the field_type classes a few releases ago was to centralise all the information about that field_type and avoid special-case logic littered throughout BP

I agree with this in principle. But richtext is a feature that (a) might be shared by more than one field type, and (b) should be toggle-able (at least via filter) for individual fields, so that you can have a specific textarea field with richtext disabled. As for (a), we can probably squeeze some sort of richtext logic into the BP_XProfile_Field base class, though it's going to be unused by all fields but 'textarea'. As for (b), the field type objects generally don't know anything about the fields that instantiate them, so I'm not sure of a non-hack workaround for it.

DJPaul - Do you have specific suggestions for how the proposed improvement can be fit into the field_type infrastructure, without sacrificing either (a) and (b), while remaining true to the original vision of field types? I'm struggling to think of anything that seems less than arbitrary, and I don't want this very nice improvement to be held up out of aesthetic concerns over code organization.

@boonebgorges
3 years ago

#28 follow-up: @boonebgorges
3 years ago

DJPaul - Disregard most of the last comment. It's inaccurate and kinda snotty :)

The one part that's true is that richtext support should be a property of *fields*, not *field types*. It is, of course, the field type that supplies the markup; and the field type will determine the default richtext setting for a given field; but it's a setting that should ultimately be the responsibility of the field itself.

5625.3.diff modifies the approach a bit by introducing supports_richtext as a property of field types, in the same way that supports_options and other field type properties set certain defaults for fields. Then, in bp_xprofile_is_richtext_enabled_for_field(), I fall back on that value, if found. This is a bit better than the previous technique. In theory, this check might become a method on BP_XProfile_Field instead of a function, but we don't really do this anywhere else, so I figure it's an odd construction to introduce just for this feature.

I was going to suggest moving the allowed_kses customisations into the field_type somehow because, again, it's information about a field type, and not a specific instance of a field.

I haven't touched this in the latest patch. I don't necessarily agree that allowedtags is *essentially* a property of the field type, though I do think (as above) that the field type should be able to set the default allowedtags value for fields of the type. But I wasn't sure the pattern that would make the most sense, given that only textareas support richtext out of the box. Perhaps there's a BP_XProfile_Field_Type_Textarea::filter_allowed_tags() callback that does what I've currently put into xprofile_filter_kses()? How and when does the field type add_filter()? I would personally be OK with not solving this larger architectural problem for v1 of the feature, but if you have an idea for how it might look, I'd be happy to hear it.

#29 in reply to: ↑ 28 @sooskriszta
3 years ago

Replying to boonebgorges:

The one part that's true is that richtext support should be a property of *fields*, not *field types*.

Alternatively, you could consider 2 different types of text fields: simple text and rich text. In that case, the richtext support would go into field type.

Of course this depends on where you think richtext support is needed. I think it's needed only for textareas so simple text type and rich text type makes sense to me. If, however, you want rich text to be available to other field types (I struggle to think of examples) then obviously it would be absurd to create 2 versions of each.

Last edited 3 years ago by sooskriszta (previous) (diff)

#30 @DJPaul
3 years ago

  • Keywords commit added

Latest patch looks great! 🚢

#31 @boonebgorges
3 years ago

Alternatively, you could consider 2 different types of text fields: simple text and rich text. In that case, the richtext support would go into field type.

Yeah. My thinking is: (a) richtext vs non-richtext doesn't seem like a distinction on a par with the current field types (textbox, textarea, radio, etc), though I'm not sure that I could come up with a full explanation of why it doesn't :) and (b) there are so few cases in which one might legitimately want to disable richtext for a given field that I think it's appropriate to relegate it to a filter, and not clutter up the interface with it. If we go with the minimal solution proposed in this ticket - enable richtext for textarea, and let people turn it off via filter - it's easy to introduce more fine-grained defaults and UX in the future, should there be demand for it.

#32 @boonebgorges
3 years ago

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

In 10254:

Enable richtext editing for xProfile textarea fields.

Profile fields of the 'textarea' type are now edited using wp_editor().
Output escaping has been modified to whitelist all tags permitted by the
"teeny" version of the wp_editor() interface.

Richtext is enabled for all and only 'textarea' fields:

  • To enable richtext editing for a custom field type, set the supports_richtext property of your BP_XProfile_Field_Type class to true. In these cases, you'll need to provide your own editing markup as well; see the edit_field_html() and admin_field_html() methods of BP_XProfile_Field_Type_Textarea for inspiration.
  • To disable richtext editing for specific 'textarea' fields, filter bp_xprofile_is_richtext_enabled_for_field.

Props needle, boonebgorges.
Fixes #5625.

#33 @DJPaul
3 years ago

  • Keywords has-patch needs-testing commit removed

Re-opening because we need to document the addition of the field_id parameter on the blog or codex or both. The change causes the following strict standards warning on code that worked previously:

Declaration of My_Class::display_filter() should be compatible with BP_XProfile_Field_Type_Datebox::display_filter

#34 @DJPaul
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#35 @DJPaul
3 years ago

@hnla or @mercime - do we have a good codex page where we could document this change? It's not a "breaking" change but it will spam errors everywhere for developers with WP_DEBUG on.

#36 @hnla
3 years ago

Would think that would normally have been our changelog? Do we need something clearer in place?

I started off this section for template changes but not sure it's applicable for what you're needing:

https://codex.buddypress.org/themes/updating-custom-themes/

#37 @DJPaul
3 years ago

You're probably right. We can note it in our changelog.

#38 @DJPaul
3 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.