Skip to content

Conversation

@wise-king-sullyman
Copy link
Collaborator

What: Closes #6717

Additional issues:

@patternfly-build
Copy link
Collaborator

patternfly-build commented Dec 22, 2021

Copy link
Contributor

@nicolethoen nicolethoen left a comment

Choose a reason for hiding this comment

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

LGTM

I will note that if all you're doing is declaring your examples as functions like:
export const ActionListMultipleGroups = () => (...
you might not even need to do that. I think the examples will also still work without the exported const. i.e.

<ActionList>
    <ActionListGroup>
      <ActionListItem>
        ...
      </ActionListItem>
    </ActionListGroup>
  </ActionList>

Some of the simple examples do that - like Alerts

But that may be a person preference thing. just an FYI if it interests you.

@nicolethoen
Copy link
Contributor

Another note: these examples are all valid typescript, but all your function examples don't explicitly declare the return type to be a FunctionComponent. That doesn't matter since no one is importing and using them. In this case typescript does not complain because it can infer the type. But it may be a good habit to be explicit about the return type? Or at least be aware that in most other cases you need to be explicit - not necessarily in these cases.

I think both of the points I bring up here are small. I'll make note to chat about that at the retro so that we can have like some team agreement about style and such as we continue this conversion effort.

@wise-king-sullyman
Copy link
Collaborator Author

@nicolethoen those concerns actually are related to a potential small issue I've seen. The markdown parser only identifies typescript examples as typescript when it actually has to convert the code into regular js because of explicit typing.
image

Your suggestion of explicitly declaring the return type for all components would make this a non-issue, but your suggestion of just having the plain JSX would contribute to the issue.

An alternative solution of course would be to edit the md parser to identify anything as TS if it's specified as such. I'm currently poking around in the MD parsing code to try and figure out what it would take to do so.

@nicolethoen nicolethoen requested a review from jpuzz0 January 5, 2022 20:07
@nicolethoen
Copy link
Contributor

As per today's discussion, let's make sure each functional component has an explicit return type.

@nicolethoen nicolethoen merged commit f1b1c38 into patternfly:main Jan 10, 2022
@patternfly-build
Copy link
Collaborator

Your changes have been released in:

  • eslint-plugin-patternfly-react@4.23.1
  • @patternfly/react-catalog-view-extension@4.35.1
  • @patternfly/react-charts@6.37.1
  • @patternfly/react-code-editor@4.25.1
  • @patternfly/react-console@4.35.1
  • @patternfly/react-core@4.184.1
  • @patternfly/react-docs@5.45.1
  • @patternfly/react-icons@4.35.1
  • @patternfly/react-inline-edit-extension@4.29.1
  • demo-app-ts@4.144.1
  • @patternfly/react-integration@4.146.1
  • @patternfly/react-log-viewer@4.29.1
  • @patternfly/react-styles@4.34.1
  • @patternfly/react-table@4.53.1
  • @patternfly/react-tokens@4.36.1
  • @patternfly/react-topology@4.31.1
  • @patternfly/react-virtualized-extension@4.31.1
  • transformer-cjs-imports@4.22.1

Thanks for your contribution! 🎉

@wise-king-sullyman wise-king-sullyman deleted the action-list-ts-conversion branch January 10, 2022 21:30
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.

ActionList: Convert examples to TypeScript

5 participants