Skip to content

Conversation

@mvollmer
Copy link

@mvollmer mvollmer commented Oct 31, 2024

closes# #11183
Here are a few small changes to the TypeaheadSelect that we could use in Cockpit.

  • Our linter doesn't like the "any" type.
  • TypeScript actually reports a typing error for the "isDisabled" property, which seems easily fixed.
  • Not all our typeahead users specify a "onClearSelection" function, and for those the "X" button was still there but did nothing. I think it is better to not show that button in that case.

Thanks!

If there is no such function, nothing will change when clicking that
button, and it is less confusing to not show it in that case.
@patternfly-build
Copy link
Collaborator

patternfly-build commented Oct 31, 2024

This one:

# pkg/lib/cockpit-components-typeahead-select.tsx:406:6 - error TS2375: Type '{ children: Element; className?: string; isExpanded: boolean; isDisabled: boolean | undefined; isFullHeight?: boolean; isFullWidth: boolean; splitButtonOptions?: SplitButtonOptions; ... 281 more ...; ref: Ref<...>; }' is not assignable to type 'MenuToggleProps' with 'exactOptionalPropertyTypes: true'. Consider adding 'undefined' to the types of the target's properties.
#   Types of property 'isDisabled' are incompatible.
#     Type 'boolean | undefined' is not assignable to type 'boolean'.
#       Type 'undefined' is not assignable to type 'boolean'.
@mvollmer
Copy link
Author

  • TypeScript actually reports a typing error for the "isDisabled" property, which seems easily fixed.

Or not... :-) I didn't read the error message carefully enough, which is:

# pkg/lib/cockpit-components-typeahead-select.tsx:406:6 - error TS2375: Type '{ children: Element; className?: string; isExpanded: boolean; isDisabled: boolean | undefined; isFullHeight?: boolean; isFullWidth: boolean; splitButtonOptions?: SplitButtonOptions; ... 281 more ...; ref: Ref<...>; }' is not assignable to type 'MenuToggleProps' with 'exactOptionalPropertyTypes: true'. Consider adding 'undefined' to the types of the target's properties.
#   Types of property 'isDisabled' are incompatible.
#     Type 'boolean | undefined' is not assignable to type 'boolean'.
#       Type 'undefined' is not assignable to type 'boolean'.

I have pushed a hopefully better fix.

Why do you not see this error? Do we have different versions of MenuToggleProps?

@mvollmer mvollmer force-pushed the typeahead-template-improvs branch from 766a64d to cbeb8ab Compare October 31, 2024 08:17
@tlabaj tlabaj requested review from adamviktora and kmcfaul November 5, 2024 21:41
@kmcfaul
Copy link
Contributor

kmcfaul commented Nov 7, 2024

Not sure why we aren't seeing this error. Are you running with strict mode maybe?

@mvollmer
Copy link
Author

Are you running with strict mode maybe?

Yes.

We have copied the template into our sources so that we can make changes like in this PR, and we lint and type-check it like the rest of the Cockpit code. If we would import the template from @patternfly/react-templates, I guess we would not see that error.

@tlabaj tlabaj merged commit 0bdb559 into patternfly:v5 Nov 14, 2024
2 checks passed
@patternfly-build
Copy link
Collaborator

Your changes have been released in:

  • @patternfly/react-docs@6.4.16
  • @patternfly/react-templates@1.1.9

Thanks for your contribution! 🎉

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.

5 participants