PLATUI-2918 add empty value #304
Conversation
| @@ -46,7 +46,7 @@ | |||
| @defining((value.nonEmpty && item.value == value) || item.selected) { isSelected => | |||
| @* Allow selecting by text content (the value for an option when no value attribute is specified) *@ | |||
| @defining(item.value.getOrElse(item.text)) { effectiveValue => | |||
| <option @if(item.value.nonEmpty) {value="@item.value"} | |||
| <option value="@item.value" | |||
There was a problem hiding this comment.
I reckon on this one, rather than reverting the output, we should change the default value of the case class from value=None to value=Some("") - or, do nothing and just update guidance, but that leaves some risks to me of people not catching the change to get previous default behaviour
There was a problem hiding this comment.
For any future readers, Oscar and I discussed this.
Default value of Some("") doesn't resolve the problem fully here; but neither does the proposed solution to revert the change. Instead, a new placeholder attribute will be made available which will accept a string and will generate the placeholder SelectItem on behalf of users. Alternatively, providing their own Some("") can work.
| @@ -971,11 +967,12 @@ A preferred way would be to select a default value using the `selected` attribut | |||
| ).asAccessibleAutocomplete(Some( | |||
| AccessibleAutocomplete( | |||
| showAllValues = true, | |||
| autoSelect = false) | |||
| autoSelect = false, | |||
| emptyItem = Some("Select an item") | |||
There was a problem hiding this comment.
hmm, I know people said no to placeholder - but I don't think it will be clear what this param will do - a bit of me is wondering seeing this, if we're not going to ever forward this to the placeholder param of a11y autocomplete, if we should even give people an option for this rather that just having it be guidance that you need to explicitly pass an empty value to create a initial placeholder option
There was a problem hiding this comment.
emptyItem isn't an intuitive name to me, but I'm struggling to come up with a concise alternative 😅
| "data-module" -> accessibleAutocomplete.dataModule | ||
| ) | ||
|
|
||
| val dataAttributes = | ||
| toMapOfDataAttributes(accessibleAutocomplete.getOrElse(AccessibleAutocomplete(None))) | ||
|
|
||
| val maybeDataLanguage = Map("data-language" -> messages.lang.code).filterNot(_._2 == En.code) | ||
|
|
||
| select.copy(attributes = select.attributes ++ dataAttributes ++ maybeDataLanguage) | ||
| val maybeEmptyItem = accessibleAutocomplete.map(_.emptyItem).getOrElse(None) |
There was a problem hiding this comment.
getOrElse(None) is a bit weird to see - you can .flatten nested Options, but then .map(...).flatten simplifies to .flatMap(...)
| val maybeEmptyItem = accessibleAutocomplete.map(_.emptyItem).getOrElse(None) | |
| val maybeEmptyItem = accessibleAutocomplete.flatMap(_.emptyItem) |
| @@ -63,6 +63,13 @@ trait RichSelectSupport { | |||
| def withHeadingAndSectionCaption(heading: Content, sectionCaption: Content): Select = | |||
| withHeadingLabel(select, heading, Some(sectionCaption))((sl, ul) => sl.copy(label = ul)) | |||
|
|
|||
| def withEmptyItem(emptyItemText: String): Select = { | |||
There was a problem hiding this comment.
need a test for this new extension method?
| @@ -72,15 +79,26 @@ trait RichSelectSupport { | |||
| "data-show-all-values" -> accessibleAutocomplete.showAllValues.toString, | |||
| "data-default-value" -> accessibleAutocomplete.defaultValue.getOrElse(""), | |||
| "data-min-length" -> accessibleAutocomplete.minLength.map(_.toString).getOrElse(""), | |||
| "data-empty-item" -> accessibleAutocomplete.emptyItem.map(_.toString).getOrElse(""), | |||
There was a problem hiding this comment.
(where) is this data-empty-item attribute actually used?
| @@ -971,11 +967,12 @@ A preferred way would be to select a default value using the `selected` attribut | |||
| ).asAccessibleAutocomplete(Some( | |||
| AccessibleAutocomplete( | |||
| showAllValues = true, | |||
| autoSelect = false) | |||
| autoSelect = false, | |||
| emptyItem = Some("Select an item") | |||
There was a problem hiding this comment.
emptyItem isn't an intuitive name to me, but I'm struggling to come up with a concise alternative 😅
| maybeEmptyItem match { | ||
| case None => | ||
| select.copy(attributes = select.attributes ++ dataAttributes ++ maybeDataLanguage) | ||
| case Some(text) => | ||
| val emptyItem: SelectItem = SelectItem.emptyItemObject(text) | ||
| select.copy( | ||
| attributes = select.attributes ++ dataAttributes ++ maybeDataLanguage, | ||
| items = emptyItem +: select.items | ||
| ) | ||
| } |
There was a problem hiding this comment.
I would maybe restructure this to isolate the conditional empty item bit from the attributes which are the same in both cases
| maybeEmptyItem match { | |
| case None => | |
| select.copy(attributes = select.attributes ++ dataAttributes ++ maybeDataLanguage) | |
| case Some(text) => | |
| val emptyItem: SelectItem = SelectItem.emptyItemObject(text) | |
| select.copy( | |
| attributes = select.attributes ++ dataAttributes ++ maybeDataLanguage, | |
| items = emptyItem +: select.items | |
| ) | |
| } | |
| (maybeEmptyItem match { | |
| case Some(text) => select.copy(items = SelectItem.emptyItemObject(text) +: select.items) | |
| case None => select | |
| }).copy(attributes = select.attributes ++ dataAttributes ++ maybeDataLanguage) |
Purpose of PR
Value=”” is needed for an empty item in the latest play-frontend-hmrc versions (> 9.0.0). Without this, an empty item's text will be put in place of the select box, meaning users will have to delete this each time. For instance if you don't pass a value, you but pass a text of "Pick a country", this will always show rather than the preferred empty string "". There are workarounds, but without being able to have a no value box, users who don’t accept JavaScript will be impacted by this.
Easiest solution is to reintroduce value=”” to SelectItem elements. This could potentially be impacting up to 25 services, depending on what items they pass through. 6 services are definitely affected by this. We could get all 25 services to update to a different method, but then we’d need to make sure we support a workaround.
We have implemented a new parameter to AccessibleAutocomplete, emptyItem, which is of type Option[String]. This allows people to add a new item to AccessibleAutocomplete by passing
emptyItem = Some("Text here"). This is appended to the top of the list and doesn't change ordering of the rest of the list. The term placeholder was originally used, but this was deemed not accessible, as a placeholder typically refers to the example text provided to an input box.