Skip to:
Content

Opened 3 years ago

Closed 3 months ago

#5500 closed enhancement (fixed)

Date xprofile field enhancement

Reported by: sooskriszta Owned by: boonebgorges
Milestone: 2.7 Priority: normal
Severity: normal Version:
Component: Extended Profile Keywords: has-patch needs-testing
Cc: vivek@…, mercime.one@…

Description

I can't claim that this will useful for all or most communities. I do believe that many communities, especially "social networking"-type communities will find it valuable.

This could be formed by 3 dropdowns: one each for date, month and year. The sequence of the dropdowns should be dictated by the date format selected by admin in WordPress settings.

There should be 2 mutually exclusive options (checkboxes):

  • Show year in public profile (unchecked by default) - so by default the public profile shows only date and month.
  • Show age instead of date of birth (unchecked by default) - overrides previous option..if checked the profile shows calculated age in years (today - dob), instead of the date of birth

The admin should also have the right to limit registration/profile access by age. e.g. A checkbox and a textbox in admin panel
Only allow members over years of age (unchecked by default)
In frontend, the dropdowns only list dates that would make the member's age over the admin-specified minimum age. If admin has not selected any minimum, then the member's age should be at least 0 days.

Standard validation rules for dates apply.

Attachments (21)

enhanced-date-field.png (78.6 KB) - added by sooskriszta 2 years ago.
Suggested "enhanced" date xprofile field UI wireframe
buddypress-15-07-24--10-25.zip (1.9 MB) - added by axelskynet 17 months ago.
The file contains a version of the plugin and the patch file for it.
1.PNG (53.1 KB) - added by sooskriszta 17 months ago.
Date of birth (actual screenshot)
2.PNG (52.7 KB) - added by sooskriszta 17 months ago.
Age (actual screenshot)
date-field-patch#1.zip (7.1 KB) - added by abweb 8 months ago.
date_field_patch#1_abweb.patch (30.1 KB) - added by DJPaul 8 months ago.
5500.diff (19.3 KB) - added by boonebgorges 8 months ago.
5500_update.patch (21.8 KB) - added by abwebstudio 8 months ago.
Updated patch
screen_patch1_01.png (35.4 KB) - added by abwebstudio 8 months ago.
Screenshot of new UI
screen_patch1_02.png (24.8 KB) - added by abwebstudio 8 months ago.
Screenshot of working field value examples
5500_update2_abweb.patch (74.3 KB) - added by abwebstudio1 7 months ago.
Patch Update
5500_update3_abweb.patch (22.4 KB) - added by abwebstudio1 6 months ago.
5500_update3_abweb.patch
5500.2.diff (14.7 KB) - added by boonebgorges 4 months ago.
Screenshot_2016-08-17_14-22-24.png (30.5 KB) - added by boonebgorges 4 months ago.
5500.3.diff (18.1 KB) - added by boonebgorges 3 months ago.
5500.patch (8.2 KB) - added by mercime 3 months ago.
Screenshot_2016-08-31_07-00-52.png (13.7 KB) - added by boonebgorges 3 months ago.
Screenshot_2016-08-31_07-01-38.png (34.1 KB) - added by boonebgorges 3 months ago.
5500.4.diff (9.0 KB) - added by boonebgorges 3 months ago.
5500.2.patch (12.0 KB) - added by mercime 3 months ago.
5500.3.patch (3.7 KB) - added by mercime 3 months ago.

Change History (94)

#1 @boonebgorges
3 years ago

Thanks for the detailed request.

IMO, this is too specific for a field *type*. Date of Birth might be of interest to many communities, but the underlying logic - a field that allows for date input, and can display it in a number of different ways - could be used for many other purposes.

So, I'm going to propose that instead, we use this ticket to discuss ways of improving our existing Date field. I can imagine adding the following:

  • Show "time elapsed" instead of the date. This would be roughly equivalent to the "Age" feature requested above. So you'd enter "September 4, 1934" and the display value would be, say, "80 years ago"
  • Options for which dates to show. Past only, present only, specific year ranges
  • Time + date

The stuff about restricting site membership based on age/DOB does sound useful, but I think it's pretty straightforwardly plugin territory. On the bright side, if we built an enhanced Date field type, extending this to cover Age more specifically would be extremely easy.

What do you think?

#2 @sooskriszta
3 years ago

Your note makes perfect sense to me.

We should build in the "privacy" filter - don't show year - if possible, though.

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

#3 @sooskriszta
3 years ago

  • Cc vivek@… added

#4 @sooskriszta
3 years ago

  • Summary changed from Custom xprofile field: Date of birth to Date xprofile field enhancement

#5 @DJPaul
3 years ago

  • Milestone changed from Awaiting Review to Future Release

#6 @DJPaul
3 years ago

My gut feeling is many simple fields will be a better/less complicated UI than just a few field types with various options to configure behaviour (I imagine power users would appreciate the fine-grained control, but we're trying to build for the 80%).

#7 @boonebgorges
3 years ago

I agree with that idea in general, but I think that a simple toggle for setting beginning and ending dates for the date field might be an exception. It's hard to think of a use case for the date field where these settings *wouldn't* be useful. That said, I'm willing to listen to the wisdom of the crowds on this point.

#8 @DJPaul
2 years ago

See also #5646

We should agree how we want to approach this sooner rather than later, because we've had tickets from multiple people for each approach.

#9 @sooskriszta
2 years ago

I am convinced that DOB is too specific.

Let every date field have 3 additional features:

  • Show year (On by Default). If admin switches this off, then in the public front-end only date and month are shown, e.g. 1 January
  • Dates to list: year x to year y, or from x years before today to y years before today (accept negative values for x and y to allow for future dates)
  • Show time elapsed i.e. Today - Date. I differ from Boone on the language used though. I'd say simply 80 years, not 80 years ago. Here's why: Showing age: 37 years makes more sense than 37 years ago Showing countdown: If Today - Date is negative, i.e. we are counting down to an even in the future (say birthday party) then we don't want to grapple with a separate text string...we just want to say 15 days

#10 @boonebgorges
2 years ago

sooskriszta - Thanks for the feedback. But the UI you're suggesting here, as I'm envisioning it, would be several times more complex than what I think we're leaning toward. Those three additional features will translate into four or five different settings of different types. Any chance you might want to mock up how you imagine the interface looking?

The options laid out by sooskriszta are helpful in that they show all the different ways that date fields might be used. It's pushing me more in the direction of DJPaul's position that we should probably just have separate field types. That said, I do think that (eg) a DOB field still should have some settings; some sites will want to set the earliest date to be 12 years ago, while others may want it to be 21 years ago, etc. I also think there's some merit to having the option for a DOB field to display as age.

Would it maybe be helpful if we came up with three or four examples of how date fields might be used. That would help us to get a sense of where the use cases intersect, and what if anything would go into a general "Date" field in terms of settings/UI.

@sooskriszta
2 years ago

Suggested "enhanced" date xprofile field UI wireframe

#11 @sooskriszta
2 years ago

  • Keywords dev-feedback added

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


2 years ago

#13 @boonebgorges
2 years ago

#6038 was marked as a duplicate.

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


2 years ago

#15 follow-up: @johnjamesjacoby
22 months ago

A few ideas here:

  • Is there an existing date-picker JavaScript library we can lean on?
  • Is there an existing PHP library that would map date & time formats to fields with appropriate boundaries?
  • Date & time fields could have many different usages and permutations of combinations. Are there any event calendar plugins that are doing this really well we can learn from or partner up with?
  • I think I would ultimately like to see our "Date" field replaced by a more robust "Date/Time" field. One with a few popular options to choose from (date only -- all day, date & time, date range, date & time range, etc...)
  • Make it so fields are mapped to WordPress's format, and not hard coded to Day/Month/Year order

In general, I think there is good discussion happening here. Our current Date field is not ideal, but exactly what is ideal likely needs to influenced by other successful projects that use dates & times.

#16 in reply to: ↑ 15 @sooskriszta
22 months ago

Replying to johnjamesjacoby:

  • I think I would ultimately like to see our "Date" field replaced by a more robust "Date/Time" field. One with a few popular options to choose from (date only -- all day, date & time, date range, date & time range, etc...)

JJJ, I feel date (date only) and date range (event - all day, datetime range) have very different use cases. Date seems more appropriate for profile fields. While date range is more of an event-based field - perhaps more suited to a new "Events" component.
e.g. Profile field: Date of Birth
Events field: Birthday party date/time

@axelskynet
17 months ago

The file contains a version of the plugin and the patch file for it.

#17 @sooskriszta
17 months ago

  • Keywords has-patch added; dev-feedback removed

In my testing patch works well.

#18 @DJPaul
17 months ago

  • Keywords needs-testing added
  • Milestone changed from Future Release to 2.4

Thanks again for the patch. Not convinced that we should do this yet, but we'll definitely take a look at your work.

Can whoever tests this first attach some screenshots of the UI changes to this ticket? They'd be interesting to see.

#19 @sooskriszta
17 months ago

The UI is very similar to my wireframes actually.

@sooskriszta
17 months ago

Date of birth (actual screenshot)

@sooskriszta
17 months ago

Age (actual screenshot)

#20 @axelskynet
17 months ago

We have added the text note "Note: Year range is allowed between 1902 and 2037 years only." because the core has year restriction http://share.redix.ru/s/?file=andreyivanov/2015-07-25_0115.png&w=598&h=366. Even if user selects the year out of this range it will be saved but will not be shown on Frontend. BuddyPress developers can fix this issue if it will be needed.

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


17 months ago

#22 follow-up: @boonebgorges
17 months ago

I'm coming around to the idea of including these features, but I think the suggested UI needs some refinement. Some thoughts/ideas, for discussion:

  • Let's break out the options into a separate metabox - "Field Options" or something like that. Or maybe even two separate metaboxes: "Range" and "Format"
  • The year/no-year checkbox is kinda arcane. If we're going to support this, let's make a more general set of formats, so that admins can decide in a more fine-grained way how they want dates to be formatted. The easiest way to do this would be to provide a set of hardcoded formats: "February 25, 2015", "February 25", "2015-02-25", etc. More complex would be a free-form field with swappable placeholders, akin to PHP's date() (but more user-friendly). Defaults should be sensitive to localized date values/WP settings.
  • We could use better labels for the two radio buttons related to Range. I suggest something like "Fixed" and "Dynamic".
  • If we're going to make these improvements, we can remove the 1902-2037 restriction, which I'm sure was implemented to avoid problems with freeform data entry.

Thanks for your work on this so far!

#23 @DJPaul
17 months ago

The 2037 thing was to prevent bugs based on how PHP handles dates on 32bit operating systems. I know it was a UI-only "fix" because people can send whatever value in the form submission, but figured that was a harmless edge case. See https://en.wikipedia.org/wiki/Year_2038_problem (I took one year off of that for safety).

I forget why I took it down to 1902, but probably a related reason.

As long as we can dynamically find out the maximum year supported in PHP, we should be able to implement the restriction only for affected sites.

#24 in reply to: ↑ 22 ; follow-up: @sooskriszta
17 months ago

Replying to boonebgorges:

  • The year/no-year checkbox is kinda arcane. If we're going to support this, let's make a more general set of formats, so that admins can decide in a more fine-grained way how they want dates to be formatted. The easiest way to do this would be to provide a set of hardcoded formats: "February 25, 2015", "February 25", "2015-02-25", etc. More complex would be a free-form field with swappable placeholders, akin to PHP's date() (but more user-friendly). Defaults should be sensitive to localized date values/WP settings.

Can't say I understand what you mean. Could you rephrase/elaborate?

At the moment year/no-year checkbox literally does exactly what it says on the tin.
If year shows 1 Jan 2016 (based on WordPress date format), then no year shows 1 Jan.

#25 in reply to: ↑ 24 @boonebgorges
17 months ago

Replying to sooskriszta:

At the moment year/no-year checkbox literally does exactly what it says on the tin.
If year shows 1 Jan 2016 (based on WordPress date format), then no year shows 1 Jan.

Yes. But what I'm suggesting is that this checkbox is too specific to your use case for us to adopt in BuddyPress. What if I only want to show years? Only months? Only month + year? There are valid use cases for all of these. So I'm suggesting one of the following:

  1. A curated list of format options. Maybe a radio button of options like:
* February 23, 2015
* February 23
* February 2015
* February
* 2015
  1. A parameterized system for defining your desired format. Perhaps we'd define some subset of PHP's date() syntax, maybe something like this:
Enter your desired date format. Use the following placeholders for various parts of the date:
  - %F - Full month name (eg, January)
  - %m - Month number, with leading zeroes (eg, 04)
  - %d - Day number, with leading zeroes (eg, 13)
  - %j - Day number, without leading zeroes (eg, 4)
  - %Y - Year number
  etc

Format: [ %F %d, %Y ] (February 23, 2015) // this would maybe update dynamically as a preview

These are just some ideas. It would be very valuable to see what existing form-building plugins do. We don't necessarily need (or want) to build a full date-format system, but perhaps looking at examples would give us a sense of what would be a useful subset of such a system.

#26 @boonebgorges
15 months ago

  • Milestone changed from 2.4 to Future Release

I like what's happening here, but it's not ready for prime time. At a minimum, we need to have the curated list of format options, as described above. The patches here are a good start, but need a champion to be core-worthy.

#27 @sooskriszta
10 months ago

So, preferred solution:
http://i.imgur.com/wAiFiKt.jpg
?

#28 @DJPaul
10 months ago

That looks decent.

#29 @boonebgorges
10 months ago

  • Keywords needs-patch added; has-patch needs-testing removed

Oh right, I forgot that WP had this built into options-general.php. That means (a) we don't have to invent a new interface, and (b) we can use their validation. Let's do it.

buddypress-15-07-24--10-25.zip needs a refresh (and ideally to be regenerated from the buddypress directory) before it can be properly tested.

#30 @DJPaul
9 months ago

  • Keywords good-first-bug added
  • Milestone changed from Future Release to 2.6

Would be got to get this in 2.6; not a bad ticket for a new contributor who knows their way around WordPress development and wants something snack-size.

#31 @sooskriszta
8 months ago

This is what the new patch does:
http://i.imgur.com/DdHSuFR.png

#32 @DJPaul
8 months ago

I uploaded a copy of that patch extracted from the ZIP

#33 @DJPaul
8 months ago

@abweb Thanks for your patch!

It does need a lot of tidy-up and adjustments for our code style; but any progress is great! Are you interested in iterating on your patch, if we let you know what needs to be changed?

@boonebgorges
8 months ago

#34 follow-up: @boonebgorges
8 months ago

  • Keywords good-first-bug removed

5500.diff regenerates the patch from the buddypress directory, so it can be properly applied, and removes a PHP short tag that was throwing a fatal error on my installation.

A lot of the work here is really good. Aside from coding standards, the one thing that's making the patch hard to follow is the data storage. Some settings are stored in an option called BPOptRegData (which seems to set the defaults for the fields?), while other settings are stored in child fields of the date field. Neither of these seem well-suited to the purpose. I think it's probably appropriate to store all settings related to a field in a single array (which will be auto-serialized by WP) using bp_xprofile_update_meta( $field_id, 'field' ... ). Fallback values (BPOptRegData) can be defined in code.

As for the UI:

  • I am convinced that the date_format stuff is good, and should go in with just a few tweaks.
  • Idea: "Show time elapsed" should be one of the date formats instead of a separate checkbox. To make things consistent, instead of using today's date to demonstrate the formats, always use the same one - say, March 1 2012 - and then the "elapsed" option can be dynamic - "4 years" or whatever. We could also have an additional one that is formatted as "4 years ago".
  • The "range" stuff seems like a good feature, but I think it should be simplified: just have a start year and an end year. I understand why you might want to have the dynamic "ago" feature, but the UI here is confusing, and incomplete - there's no way to add future dates. Forcing the user to enter years makes it more flexible and clearer. If we absolute, positively must have more complex options here, I suggest the following option format:
() from { 1982 } to { 2023 }
() from { 34 } [ years ago ] to { 7 } [ years from now ]

where {brackets} are text inputs and [brackets] are dropdowns.

  • Another idea. If the date_format selected doesn't include year (or month or day), we shouldn't include it in the front-end interface.

Is this sufficient feedback for another go-round? My gut feeling is that we will have a better chance of getting something done for 2.6 if we scale back a bit: if we, say, do the date format stuff first. But if @sooskriszta and @abweb feel that it's possible to do it all at once, let's go for that :)

#35 in reply to: ↑ 34 @sooskriszta
8 months ago

Replying to boonebgorges:

  • Idea: "Show time elapsed" ... there's no way to add future dates

I had imagined putting negative numbers for future dates, but I can see how this could be confusing to some BuddyPress admins

@abwebstudio
8 months ago

Updated patch

@abwebstudio
8 months ago

Screenshot of new UI

@abwebstudio
8 months ago

Screenshot of working field value examples

#36 @abwebstudio
8 months ago

I have uploaded new version of this patch.

  1. Fixed UI. Now it is more standard for WP.
  2. Added changes in range settings.
  3. It was fixed a bug when creating a field value. They just were not created. The problem was the method is_valid() absence and in the code at this place:"// Turn the concatenated value into a timestamp. "

Happy testing. I hope you will enjoy. Waiting for an answer

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


8 months ago

#38 follow-up: @boonebgorges
8 months ago

Thanks very much, @abwebstudio. I think the UI is getting close to being very good. There are a few things I would change with the wording ("Custom" instead of "Arbitrarily"; add a link to date formatting rules, like WP has; "Range" instead of "Year Range"; etc) but I really like the general workflow here. Very powerful, and quite usable.

The internals of the patch still need lots of reworking, in accordance with some of the comments I made above about coding standards, data storage, and so forth. In addition to this, I'd like to consider a few more pieces before considering for merge:

  • It would be nice to have unit tests for some of the range stuff. We don't need extensive tests for PHP's date format functions, but the range stuff is custom, so I think it's worth testing.
  • In Edit mode, Month should only be shown when Month will show in the interface, etc.
  • Validation for custom date formats. WP does this using AJAX, in the form of a preview - it doesn't tell you when you've entered an "invalid" date format, since there's no real "invalid" - unrecognized characters are interpreted by date() as literals. I think we should copy what WP does here.

@abwebstudio If you're willing to continue working on these items, it'd get this ticket done faster. Otherwise I'm interested in shepherding it along myself, though it may not happen for this release if it's solely in my court.

Thanks for your continued work! Cool feature!

#39 in reply to: ↑ 38 ; follow-up: @sooskriszta
8 months ago

Replying to boonebgorges:

If the date_format selected doesn't include year (or month or day), we shouldn't include it in the front-end interface.

@boonebgorges I don't have a specific use-case in mind, and this may be inapplicable for this particular ticket, but I'd like to see in BuddyPress a decoupling of the input data and the output information.

#40 in reply to: ↑ 39 @boonebgorges
8 months ago

Replying to sooskriszta:

Replying to boonebgorges:

If the date_format selected doesn't include year (or month or day), we shouldn't include it in the front-end interface.

@boonebgorges I don't have a specific use-case in mind, and this may be inapplicable for this particular ticket, but I'd like to see in BuddyPress a decoupling of the input data and the output information.

I see this point, and I guess I don't feel strongly about my suggestion. My main thinking is that it's awkward to ask someone for, say, their birthday, knowing that you're not going to show the year on the front end, but requiring that the user provide it when editing the profile. I guess in some ideal world (though is this really ideal? seems like a potentially very confusing interface) the site admin would be able to say "collect the year, but don't display it", or "make the year optional" or "require the day and month, make the year optional, and only display the month and year" or whatever. Maybe, as a compromise, the validation should work like this: fields that are not going to be displayed on the front end should be de facto optional - that is, the validation routine will not throw an error if you don't include a Year, if the year is not part of the display format.

#41 @DJPaul
8 months ago

@abwebstudio I spend a few hours helping you get part of this patch more ready. Are you developing this change on Github (doesn't matter if you are not) and, more important, what would you like me to tackle? Hopefully more precise than "everything". ;)

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


7 months ago

#43 @boonebgorges
7 months ago

  • Milestone changed from 2.6 to Future Release

I'm a big fan of the work done in this ticket so far, but we're too close to our 2.6 beta to do the necessary work to get it across the finish line. @abwebstudio, if you can respond to @DJPaul's requests above, we can look at it again for 2.7.

#44 @sooskriszta
7 months ago

@boonebgorges @DJPaul What changes are need to the current patch? Specifically, what changes from https://buddypress.trac.wordpress.org/ticket/5500#comment:38 are required before this can be merged?

Perhaps I can try and persuade @abwebstudio to complete that.

@abwebstudio1
7 months ago

Patch Update

#45 @abwebstudio1
7 months ago

Update for patch ready. Please test it. (5500_update2_abweb)

#46 follow-up: @boonebgorges
7 months ago

Hi @abwebstudio1 - Thanks for the updated patch. Could you try regenerating it? All the characters have been HTML encoded, and there's a huge block of code from what appears to be some sort of Slack client at the top.

#47 in reply to: ↑ 46 @abwebstudio1
7 months ago

Replying to boonebgorges:

Hi @abwebstudio1 - Thanks for the updated patch. Could you try regenerating it? All the characters have been HTML encoded, and there's a huge block of code from what appears to be some sort of Slack client at the top.

Hi @boonebgorges .

I will fix this issue ASAP.

@abwebstudio1
6 months ago

5500_update3_abweb.patch

#48 @abwebstudio1
6 months ago

Update for patch ready. Please test it. (5500_update3_abweb)

#49 @abwebstudio1
6 months ago

  • Keywords needs-patch removed

Hello everyone. Thank you for your feedback. We were told that our patch did not incorporate the comments about “WordPressishness” (or “BuddyPressishness”) of the strategies used for storing data, etc.
Could you help us improve our code according to the WP conventions, by mentioning 3 specific instances within our patch, where we went wrong and describe how it should have been done. Our top 3 incompatibilities with WP conventions for coding standards.
Thanks

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


4 months ago

@boonebgorges
4 months ago

#51 @boonebgorges
4 months ago

  • Keywords has-patch needs-testing added
  • Milestone changed from Future Release to 2.7

5500.2.diff reworks the most recent patch from @abwebstudio1 for better accordance with coding standards and other WP/BP conventions. @abwebstudio1, I hope I haven't stepped on your toes too much by doing this myself - I thought it would be instructive for both of us ;) Here's a summary of changes I made:

  • Instead of storing some options as field children, I've moved them to xprofile meta.
  • I've overhauled the routines for validating and fetching settings, and renamed settings to make the naming conventions clearer.
  • Made strings localizable.
  • Some improvements to markup in the Edit form.
  • Moved inline scripts and styles to enqueued files.
  • Removed some parts of the patch that appear to be left over from an earlier implementation (stuff like 'show-time').
  • Removed some purported bugfixes. One was related to the 2038 bug. The other appears to be related to the way that d/m/y dropdowns are converted to date-time and then saved on profile edit. For one thing, I'm not convinced that either one fixes a real bug, or at least doesn't fix in quite the correct way. (For example, 2038 is only a problem on 32-bit systems.) More importantly, they don't seem to be directly related to this ticket, so I'd like to handle them separately if they are indeed issues.

Most of the functionality is the same. A few small changes:

  • Instead of "example" text after the custom format, I've mirrored what WP does on Settings > General, and render an example dynamically. It looks like you had originally done it this way, but abandoned it for some reason.
  • Changed some styling.
  • Moved the Documentation link so it's right after the 'Date format' block.

Looking for general feedback on the following:

  1. I feel that the 'Date format' section is pretty good, but I feel less confident that the Range interface makes sense. Does the difference between the two radio buttons make sense to others? Is there a better way to do this?
  2. Related: the 'years ago'/'years from now' dropdown poses some localization problems. I'd like feedback from a polyglot on how they're implemented. I'm pretty sure they'll work for French and Russian (two languages I speak well enough to judge) but I have no idea about other languages.
  3. I made a few small mods to the BP_XProfile_Field(_Type) API to make it all work. Specifically, I introduced BP_XProfile_Field::admin_settings_save(), which is called during the admin save routine; this method then calls admin_settings_save( $field_id ) on the field-type object. I figure that this is a good way to suggest the convention that field-types should be responsible for how their settings get saved. (BP_XProfile_Field_Type() has a stub admin_settings_save(), which should be overridden by subclasses.) @DJPaul Does this approach make sense to you?

#52 @DJPaul
4 months ago

I'll try to make time to look at this properly over the weekend, but in reply to your question:

Field Type is intended to represent a generic type of profile field, rather than a specific instance of a field. A Field Type object has no value (the value each member inputs) -- its methods that output HTML use the templating loop -- so it having a mechanism to store data doesn't seem appropriate.

But. When I was looking at #7162, which involves e.g. creating a special Field Type that maps to WordPress' authors' "biography" field, I found some problems with retrieval of some of this extra data (prob. not relevant to this ticket) and I also had concerns about a Field Type needing to have a place to save the extra data.

As this problem has come up twice, we do need to do something. Whether that's change Field Type or not, I need to more time to consider this and hopefully come up with a suggestion.

#53 @boonebgorges
4 months ago

Field Type is intended to represent a generic type of profile field, rather than a specific instance of a field. A Field Type object has no value (the value each member inputs) -- its methods that output HTML use the templating loop -- so it having a mechanism to store data doesn't seem appropriate.

Right. But fields might have settings or various manners of being customized that are specific to the field type. It makes sense that any field of the Date type would have "range" properties, but that doesn't make sense for any other field type. The Field Type should not be aware of any specific field, but this is why I've written BP_XProfile_Field_Type::admin_save_settings() to require a $field_id parameter. So the logic is:

  • The field save controller is xprofile_admin_manage_field(), which assembles a $field object from the values in $_POST and saves it
  • Once it's set up, I call $field->admin_save_settings() (this is on the *field* object)
  • BP_XProfile_Field::admin_save_settings() does nothing but call $this->type_obj->admin_save_settings( $this->id );
  • $_POST information is extracted as required in BP_XProfile_Field_Type_Datebox::admin_save_settings()

So I think the separation of concerns is pretty good - the Type object only knows about fields in general. The icky part is accessing the $_POST global inside of the Type object. An alternative would be to put all the type-specific settings inside a single $_POST key, so that you xprofile_admin_manage_field() could pass the $_POST['type_settings'] array to the field object. But we already access $_POST in the datebox field type, when processing edit_field_options().

#54 @mercime
4 months ago

  • Cc mercime.one@… added

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


4 months ago

@boonebgorges
3 months ago

#56 @boonebgorges
3 months ago

5500.3.diff makes two main changes to address problems raised during last week's meeting:

  1. In the Dashboard, when selecting the 'datebox' field type and then selecting another field type, the field's settings weren't disappearing. The current code decides which fields to hide based on an array called supports_options_field_types, since the assumption is that only field types with "options" will need to have a settings section. I changed this to do_settings_section_field_types, and added a do_settings_section() method to the field type object; if the field type doesn't declare a do_settings_section value explicitly, it falls back on supports_options. @mercime Could you test that this fixes the bug? @DJPaul Could you think about this for a moment and let me know whether you think it's a reasonable way of handling the presence of settings sections for field types?
  1. I've removed the references to $_POST in the admin_save_settings() method of the Datebox field type. Instead, I've organized the form so that all settings are in an array field_settings; xprofile_admin_manage_field() then passes the field_settings array through to admin_save_settings(). @DJPaul I'd appreciate your review of this, too.

#57 @DJPaul
3 months ago

@boonebgorges do_settings_section_field_types etc - looks good. I understand why you're using null (implicitly) to see if the value is not set. How about setting the default to null explicitly to match the other properties, and also update the PHPDoc block? It's not always a bool.

In admin_save_settings, the default block section:

1) wp_unslash already happens in the change in xprofile_admin_manage_field where we call that function.
2) New values for settings other than range_relative_start and range_relative_end are not being sanitised... or, is the later call to validate_settings meant to take care of that? If so, maybe add a comment explaining where it happens, because I first read that intval section as the input sanitisation. I only noticed validate_settings on my third read-through.

(If I'm following that right!)

In admin_new_field_html, there's some <br> being used presumably to space out the inputs. This is not semantically correct, and we should remove those and add custom CSS instead. (Also, we can remove the inline margin-top on one of the elements).

Also in admin_new_field_html, the string 'From %s to %s' needs attention. It needs a translation comment explaining what the %s values are. I also have no idea how that kind of structure will work for things like screen readers, but we can test after. I assume this was copied from WordPress, but that doesn't mean its good. :D

In get_date_formats, the date string is missing a textdomain.

Other than those niggles, I think this is ready to go!

#58 @boonebgorges
3 months ago

@DJPaul Thanks for the careful review! All your points are good ones.

You're correct about validate_settings(). It doesn't really sanitize in the SQL-injection sense (this happens in the meta functions) but it does ensure that the passed values are valid for the given fields. I've left a comment to that effect.

I'm going to commit the first pass. @mercime If you have a few moments, an accessibility review of the admin interface would be great - especially the "from/to" dialogs.

#59 @boonebgorges
3 months ago

In 11041:

XProfile: Introduce new settings for Datebox field type.

These new settings are designed to make it easier for site administrators
to decide how date-based data will be collected and displayed.

  • "Date format" allows the admin to select how dates will be displayed on public profiles. A number of default custom date formats are included. Admins may also enter a custom date format, or opt to display time elapsed ("4 years ago", etc).
  • "Range" allows the admin to set the start and end values for the Year dropdown. There are two ways to configure the range. The first is by defining a specific set of start and end years. The second is to define a relative range: *from* a certain number of years ago (or in the future) *to* a certain number of years ago (or in the future).

Props abwebstudio, abweb, sooskriszta, boonebgorges, DJPaul, mercime.

See #5500.

#60 @mercime
3 months ago

@boonebgorges confirming issue on new date fields appearing in other field types has been resolved. Cheers :) Working on the form fields at this time.

#61 @boonebgorges
3 months ago

In 11043:

XProfile: Scrutinizer fixes after [11041].

  • Add missing documentation for get_date_formats().
  • Rmeove unused variable assignment.

See #5500.

@mercime
3 months ago

#62 @mercime
3 months ago

@boonebgorges Patch attached for your comments

  • adds labels and attributes to some form fields
  • restructures some date fields
  • improves mobile view of dates form

Screenshot of restructured fields
https://cldup.com/ZGaCdIAN2X.png

Screenshot of mobile view - before and after
https://cldup.com/Ni0YjjlhmO.png

Last edited 3 months ago by mercime (previous) (diff)

#63 @DJPaul
3 months ago

Nice work, @mercime! I think this is all good.

If we go with the reworded "Start" and "End", I still think those two could benefit from a translator comment explaining its context.

@boonebgorges This was in the previous commit, but I noticed for the form elements name properties, you've used underscores to separate the words instead of a hyphen. I know convention for CSS class names is to use a hyphen -- I vaguely remember @johnjamesjacoby going on a mass search/replace during the early years of BuddyPress -- but is it proper WordPress convention to use the underscore for form names?

I don't really know what this is called so it's hard to search for, but I couldn't easily see any mention on the WordPress Coding Standards document, so figured i'd ask. Thanks!

@boonebgorges
3 months ago

#64 @boonebgorges
3 months ago

Thank you, @mercime! This is great feedback, and the changes look good.

One friendly amendment. I understand the motivation behind adding (visible) labels to the Absolute and Relative radio buttons. But the way they were laid out in your patch didn't feel like it flowed to me. I took a swing in 5500.4.diff at compressing the space a little bit. This also allowed me to do some vertical alignment, and match the "Start" and "End" labels a bit. Screenshots at various window widths are above. What do you think?

With this new format, I feel like the word "Year" is unnecessary; the fact that the default values are years should be explanation enough. Does this seem right? If not, we can certainly add it back in. Or if you think it primarily needs to exist for screen readers, maybe it can be put in screen-reader-specific text? I'm happy to go with what you think is best - I just wanted to push one last time for what I think is the more aesthetically pleasing option :)

@DJPaul I don't know why I went with underscores. I seem to recall briefly thinking about it, and seeing that this particular form had a mix, so I went with what seemed "cleaner" to send through $_POST. I don't think WP has an explicit style recommendation on this. If you want to go with hyphens (which is what I gather from your comment), I'll make the change. Let me wait until I merge @mercime's changes, so I don't have to deal with stale patches.

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


3 months ago

#66 @mercime
3 months ago

Screenshots at various window widths are above. What do you think?

Thank you @boonebgorges. Looks great!

Attached patch adds label tag to the custom date format field, add aria attributes for descriptions of fields, move link "Documentation on date and time formatting" into the fieldset, plus style for select field for 782px and below.

@mercime
3 months ago

#67 @boonebgorges
3 months ago

Thank you, @mercime! I learned something new by reading your patch (aria-describedby).

#68 @boonebgorges
3 months ago

In 11061:

XProfile: Improvements to markup for Date settings panel.

  • Improved appearance at narrow widths.
  • Add missing labels and aria attributes.

Props mercime.
See #5500.

#69 @boonebgorges
3 months ago

In 11062:

XProfile: Use field-settings instead of field_settings for name of field settings field.

See #5500.

#70 @boonebgorges
3 months ago

@DJPaul In [11062] I changed $_POST['field_settings'] to $_POST['field-settings']. I've left the subkeys in that array with underscores, though, because that's how they're saved in the database; it seemed bad to me to have to juggle them when converting to DB-safe values.

#71 @mercime
3 months ago

Thank you @boonebgorges :) I was reviewing the commit and I noticed that in one instance of aria-describedby, my values do not match, my bad :( Attaching new patch for the aforementioned and sneaks in a fix to the width of the Type select field for mobile view per screenshot above https://cldup.com/Ni0YjjlhmO.png.

@mercime
3 months ago

#72 @boonebgorges
3 months ago

Seems good to me. Thanks, @mercime! Let's wrap this one up.

#73 @boonebgorges
3 months ago

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

In 11079:

XProfile: Improvements to responsive styling and accessibilty on date format admin panel.

Props mercime.
Fixes #5500.

Note: See TracTickets for help on using tickets.