-
Notifications
You must be signed in to change notification settings - Fork 51
Form::Fieldset
, Form::Header
, Text::Display
, Form:SuperSelect
, Form::TextInput
: small type fixes
#3008
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
1ca37e0
to
4599060
Compare
4599060
to
17092d0
Compare
Form::Fieldset
: add constant export for layout optionsForm::Fieldset
, Form::Header
, Text::Display
, Form:SuperSelect
, Form::TextInput
: small type fixes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes TypeScript typing issues across several form and text components and refines the @resultCountMessage
argument in the single/multiple super-select components to cleanly accept either a string or a function.
- Export and cast enums’ values arrays to their proper literal types (
HdsTextSizes[]
,HdsFormTextInputTypes[]
,HdsFormHeaderTitleTags[]
,HdsFormFieldsetLayouts[]
). - Omit the inherited
resultCountMessage
fromPowerSelectSignature
and redefine it as a union of string or function in both single and multiple super-select bases. - Update Handlebars templates to wire up only the function variant to PowerSelect and add the corresponding changeset.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
packages/components/src/components/hds/text/display.ts | Cast AVAILABLE_SIZES to HdsTextSizes[] |
packages/components/src/components/hds/form/text-input/base.ts | Typed TYPES as HdsFormTextInputTypes[] |
packages/components/src/components/hds/form/super-select/single/base.ts | Omit and redefine resultCountMessage in the single base signature and add getters |
packages/components/src/components/hds/form/super-select/single/base.hbs | Wire up function variant of resultCountMessage to PowerSelect |
packages/components/src/components/hds/form/super-select/multiple/base.ts | Same signature and getters adjustments for multiple base |
packages/components/src/components/hds/form/super-select/multiple/base.hbs | Wire up function variant of resultCountMessage to PowerSelect |
packages/components/src/components/hds/form/header/title.ts | Typed AVAILABLE_TAGS as HdsFormHeaderTitleTags[] |
packages/components/src/components/hds/form/fieldset/index.ts | Export typed LAYOUT_TYPES for fieldset layouts |
.changeset/fresh-horses-wink.md | Added changeset entry for the super-select fix |
Comments suppressed due to low confidence (3)
packages/components/src/components/hds/form/fieldset/index.ts:24
- [nitpick] To align with existing constants like
AVAILABLE_SIZES
andAVAILABLE_TAGS
, consider renamingLAYOUT_TYPES
toAVAILABLE_LAYOUTS
orAVAILABLE_LAYOUT_TYPES
for consistency.
export const LAYOUT_TYPES: HdsFormFieldsetLayouts[] = Object.values(
packages/components/src/components/hds/form/super-select/single/base.ts:57
- Consider adding unit tests to cover both the
string
andfunction
cases forresultCountMessageFunction
in the single super-select component to ensure this branching logic is verified.
get resultCountMessageFunction() {
packages/components/src/components/hds/form/super-select/multiple/base.ts:68
- Add tests for the
resultCountMessageFunction
getter in the multiple super-select to validate that it correctly returns a function or undefined as intended.
get resultCountMessageFunction() {
packages/components/src/components/hds/form/super-select/single/base.hbs
Show resolved
Hide resolved
packages/components/src/components/hds/form/super-select/multiple/base.hbs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor / non-blocking comments but lgtm!
packages/components/src/components/hds/form/super-select/multiple/base.ts
Outdated
Show resolved
Hide resolved
packages/components/src/components/hds/form/super-select/single/base.ts
Outdated
Show resolved
Hide resolved
91e0e8a
to
c6be203
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left a couple of comments
📌 Summary
If merged, this PR would fix some type issues found while converting the forms routes to TypeScript.
Form::Fieldset
: export an array of the layout optionsForm::Header::Title
: fix the type of the exported array of available tagsForm::TextInput
: fix the type of the exported array of available input typesText::Display
: fix the type of the exported array of available text sizesIt also fixes a type issue with the
@resultCountMessage
argument for theForm::SuperSelect::Single
andForm::SuperSelect::Multiple
.Before the fix, the type for

@resultCountMessage
was((resultCount: number) => string) & string
because the type was inherited fromPowerSelectSignature['Args']
and also set in the interface, see:This made it so the type was never correct if you set it to a string or a function.
The fix was to omit the arg from being inherited and only setting the type in the interface, see:

This type error happened because our custom after options component expects a string (link), while the actual power select component expects a function.
To resolve this, I just made different getters for each that return either a string or a function and then can be passed to the right place.

🔗 External links
Jira ticket: HDS-5101
👀 Component checklist
💬 Please consider using conventional comments when reviewing this PR.
📋 PCI review checklist
Examples of changes to controls include access controls, encryption, logging, etc.
Examples include changes to operating systems, ports, protocols, services, cryptography-related components, PII processing code, etc.