Skip to:
Content

Opened 6 months ago

Closed 4 months ago

#7665 closed enhancement (fixed)

Date Picker - make field selection names clearer

Reported by: Venutius Owned by: DJPaul
Milestone: 3.0 Priority: normal
Severity: normal Version:
Component: Extended Profile Keywords: 2nd-opinion
Cc:

Description

When you install the date selector as a profile field, it is displayed in the user registration page with each of the three fields shown as "---", but there is no indication as to which is Day, Month and Year.

Whilst the user can work this out by clicking on the fields, it's less than intuitive. a suggested fix would be to replace "---" with Day, Month and Year thus guiding the user as to what input is required.

Attachments (2)

7665.patch (3.6 KB) - added by mercime 6 months ago.
if second option chosen
7665-2.patch (6.4 KB) - added by DJPaul 6 months ago.

Download all attachments as: .zip

Change History (19)

#1 in reply to: ↑ description @Venutius
6 months ago

  • Type changed from defect (bug) to enhancement

Replying to Venutius:

When you install the date selector as a profile field, it is displayed in the user registration page with each of the three fields shown as "---", but there is no indication as to which is Day, Month and Year.

Whilst the user can work this out by clicking on the fields, it's less than intuitive. a suggested fix would be to replace "---" with Day, Month and Year thus guiding the user as to what input is required.

#2 @Venutius
6 months ago

Sorry thought I'd selected enhancement first time round.

#3 @DJPaul
6 months ago

  • Milestone changed from Awaiting Review to 3.0
  • Owner set to DJPaul
  • Status changed from new to assigned

Great idea, @Venutius! Thanks for sharing.

#4 @mercime
6 months ago

Easiest way to help members is by adding a description to the date fields, like "e.g. 28 April 2017"
https://cldup.com/XHPsf9lcs0.png

The other way is to remove the bp-screen-reader-text and screen-reader-text classes surrounding the <label> for the front and back end screens. While the change doesn't affect the backend screens--xProfile and Extended Profile-- it does affect the front end as <labels> are displayed as a block in BP Legacy and BP Nouveau.

Extended Profile Screen
https://cldup.com/kJYyf4MXNd.png

xProfile Screen
https://cldup.com/WFWkSnF7Ib.png

Front end - <labels> are displayed as a block
https://cldup.com/et0obHubpS.png

The first option mentioned is safe layout-wise if hopefully, translators take care of the new text there before 3.0 rolls out. Otherwise, there'll be english words before the day, month, year selections.

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

@mercime
6 months ago

if second option chosen

@DJPaul
6 months ago

#5 @DJPaul
6 months ago

I asked @rianrietveld about the select/option element question, in the context of screen readers: "If the text of the label and the first option is the same, (VoiceOver) only announces it once. Leaves you with the possibility that people can select and submit the first option." I was also told that "hiding the label and using the first item as placeholder/label is in fact a hack", so this is a good opportunity to correct this.

I think using block elements here is going to be too big a change for most themes to support, so displaying them inline (albeit with the label now showing) is least invasive. I also prefer how it looks. I have run with @mercime's patch and made 7665-2.patch which adds CSS fixes for the above.

Outstanding tasks for feedback:

1) Do we change --- to Day, Month, Year (on each label appropriately), so we think the VoiceOver screen reader won't say (based on above screenshot) "Date of Birth, Day, dash dash dash" -> "Date of Birth, Day".

...or...

Do we come up with a phrase to replace ---, perhaps "Unset" (needs to be something short, but basically "No year selected"), so the VoiceOver might go "Date of Birthday, Day, Unset" or "Date of Birthday, Day, No year selected".

I prefer the latter though we'd have to use a very short text otherwise we will force the width of the select boxes to grow -- typically German is my go-to example because its words tend to be very long).

2) I'm not sure "bp-datefield" is the best idea of a CSS class here. @hnla can I use "inline-label" as a class name or do you have a better idea?

#6 @hnla
6 months ago

Not sure I mind bp-datefield it is however quite specific we perhaps need a more generic class that can be used to hook styles on inline-label perhaps goes too far the other way, too generic? Maybe xprofile-label?

#7 @hnla
6 months ago

Overall though the question of a default option is a thorny one, option elements are meant to be selectable values not labels, suggestions such as setting to disabled not really a satisfactory approach.

I'm not for doing much for legacy here due to upsetting layouts - the only true option is to display labels, but we should revisit the Nouveau template and remove the screen reader classes.

#8 @DJPaul
6 months ago

The relevant parts of the template is in core files, we shouldn’t distinguish there based on template pack. I’ll probably test this on the handful of themes that I have.

#9 @hnla
6 months ago

The relevant parts of the template is in core file

True, good point. It's a sadness though, it's one of those areas I would like to unpick and build better, it's always felt a struggle dealing with those edit profile functions.

#10 @hnla
6 months ago

Find myself once more trawling around those xprofile field type classes :(

Specifically edit_field_html() we can pass an optiona= $raw_properties to this function and render form control attr however the function is responsible for more than just the controls e.g the label elements where we hard code the 'screen-reader-text' It would help if we expanded the $args passed through to include ? 'show_label' => true or whatever way round makes sense as a default state. This way I can enable the labels visually for Nouveau which I think I would prefer tbh, at the least we have an option to play around with.

#11 @hnla
6 months ago

<digressing-rambling> However an issue I have always had with these edit fields is the manner in which they have been separated into class functions so changes need to be made across multiple files, then when it comes to working in the template as we loop things I have access to only one instance of $field_type->edit_field_html( array( 'show_label' => true ) ); so have a problem identifying what field I want to target. It has always felt like xprofile was written for the convenience of the backend not frontend.

Edit: just re-factored the datebox class method to take an additional param and changed the hardcoded 'screen-reader-text' class to a var, this at least now allows me the option in the template to turn on/off, that's one class though, the problem remains that I would need to re-factor all class functions to accept same param thus all profile fields would be shown/hidden without being able to be selective.

Last edited 6 months ago by hnla (previous) (diff)

#12 @hnla
6 months ago

Wrong ticket.

Last edited 6 months ago by hnla (previous) (diff)

#13 @DJPaul
5 months ago

I need to test but if this change is accepted, I might change the proposed patch to use #buddypress label.bp-datefield instead of #buddypress .standard-form label.bp-datefield so the overall selector weight is less.

#14 @DJPaul
5 months ago

  • Keywords 2nd-opinion added

#15 @mercime
4 months ago

  • Component changed from Core to Extended Profile

I might change the proposed patch to use #buddypress label.bp-datefield

+1. Tested on Windows. Looks good.

#16 @hnla
4 months ago

Lets commit this then, I did attempt to check over myself but browser vendors saw fit to wreck my localhost dev sites so can't localhost dev until I figure out how to tell myself who I am.

I'm in favour of using labels as intended though, Nouveau we can change with impunity at this stage and we'll adjust layout if necessary.

#17 @DJPaul
4 months ago

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

Fixed in r11905

Note: See TracTickets for help on using tickets.