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

Improvements to the TypeaheadSelect template #11153

Open
wants to merge 3 commits into
base: v5
Choose a base branch
from

Conversation

mvollmer
Copy link

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
Contributor

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?

@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.

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.

4 participants