Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PLATUI-2897 enable passing of attributes to input form group wrappers #288

Merged
merged 17 commits into from Mar 28, 2024

Conversation

ellamdav
Copy link
Contributor

@ellamdav ellamdav commented Mar 21, 2024

  • add new FormGroup model
  • uplift the following inputs to add attributes to govuk-form-group:
    ** GovukCharacterCount
    ** GovukCheckboxes
    ** GovukDateInput
    ** GovukFileUpload
    ** GovukInput
    ** GovukRadios
    ** GovukSelect
    ** GovukTextarea
    ** HmrcCharacterCount
  • extract GovukFormGroupSnippet to de-duplicate repeated code across inputs
  • extract GovukHintAndErrorMessageSnippet to de-duplicate existing hint/error-message snippets repeated across inputs

@@ -27,5 +26,5 @@ import play.api.http.HttpErrorHandler
* [RuntimeException: java.lang.NoSuchMethodError: controllers.ReverseAssets.versioned(Ljava/lang/String;)Lplay/api/mvc/Call;]
* when using Assets.
*/
@Singleton
@Singleton // TODO migrate off deprecated AssetsBuilder once we drop Play 2.8 support
Copy link
Contributor Author

@ellamdav ellamdav Mar 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to remind people (like me 😅) who see the deprecation warning that we can't do this just yet 🙃

idPrefix <- Gen.option(genAlphaStr())
name <- genNonEmptyAlphaStr
nItems <- Gen.chooseNum(0, 5)
items <- Gen.listOfN(nItems, arbCheckboxItem.arbitrary)
classes <- genClasses()
attributes <- genAttributes()
} yield Checkboxes(
values <- Gen.listOfN(nItems, genAlphaStr())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noticed values was missing here 🙃

@JoPintoPaul
Copy link
Contributor

Out of interest, I notice a pattern of using fold on options, passing through the default as the first param list - was that just a matter of taste or is there nuance I'm missing there?

@JoPintoPaul
Copy link
Contributor

Also, one more comment from me - as a thought on using snippets to compose HTML templates, one thing to consider is that manually syncing with govuk-frontend templates will therefore require switching between files to compare. Obviously that's balanced against repetition / DRY, I just wanted to point out the risks with this divergence.

@ellamdav
Copy link
Contributor Author

Out of interest, I notice a pattern of using fold on options, passing through the default as the first param list - was that just a matter of taste or is there nuance I'm missing there?

TBH I just went with what was already there, but could change to .map(...).getOrElse(...) if we think that's easier to grok?

@ellamdav ellamdav marked this pull request as ready for review March 27, 2024 13:26
@ellamdav ellamdav force-pushed the PLATUI-2897-form-group-attributes branch from d71dce9 to 92cf552 Compare March 27, 2024 13:28
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some of the composition of stuff in twirl components is so hard to follow 😵

this isn't me saying do this, just me taking a moment to think if I could imagine the a flavour I might like, this kind of structure was my first thought

@(checkboxes: Checkbox)

@govukFormGroup(checkboxes.formGroup) {
  @govukFieldset(checkboxes.fieldset) { 
    @govukHint(checkboxes.hint)
    @govukErrorMessage(checkboxes.errorMessage)
    ... inputs
  }
})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed - the issue seems to be accumulating the right aria-describedby when there's maybe a supplied one, maybe one related to the hint, and maybe one related to the error message... But perhaps there's a nicer way to do it, like using cats State or something 😅

oscarduignan
oscarduignan previously approved these changes Mar 27, 2024
@ellamdav ellamdav force-pushed the PLATUI-2897-form-group-attributes branch from cb3d046 to bd0d7b6 Compare March 27, 2024 18:20
@ellamdav ellamdav merged commit 12dd4f7 into main Mar 28, 2024
1 check passed
@ellamdav ellamdav deleted the PLATUI-2897-form-group-attributes branch March 28, 2024 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants