Skip to content

chore(Popover) convert examples to TypeScript #7770

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.

Sign up for GitHub

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

Jump to bottom
Merged
tlabaj merged 1 commit into from
Aug 18, 2022
Merged

chore(Popover) convert examples to TypeScript #7770

tlabaj merged 1 commit into from
Aug 18, 2022

Conversation

@dominik-petrik
Copy link
Contributor

@dominik-petrik dominik-petrik commented Jul 28, 2022

What: Closes #7751

@patternfly-build
Copy link
Collaborator

patternfly-build commented Jul 28, 2022 •
edited
Loading

Preview: https://patternfly-react-pr-7770.surge.sh

A11y report: https://patternfly-react-pr-7770-a11y.surge.sh

@dominik-petrik dominik-petrik requested a review from tompsota July 28, 2022 15:22
@nicolethoen
Copy link
Contributor

nicolethoen commented Jul 29, 2022

It looks like the build is failing because you have an extra " before an isBeta flag on one of your examples?
either that or it's unable to parse the isBeta before the file name.

@tompsota
Copy link
Contributor

tompsota commented Aug 1, 2022

It looks like the build is failing because you have an extra " before an isBeta flag on one of your examples? either that or it's unable to parse the isBeta before the file name.

Seems like there is missing " on L61, the isBeta flag before filename shouldn't cause any problems. Although it is better to follow this syntax: ts file='<<filename>>' isBeta

thatblindgeye
thatblindgeye requested changes Aug 1, 2022
View reviewed changes
Copy link
Contributor

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

This is looking good! Had some comments below for changes

tompsota
tompsota suggested changes Aug 3, 2022
View reviewed changes
Copy link
Contributor

@tompsota tompsota left a comment

Choose a reason for hiding this comment

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

Good job! :) Just have some comments.

packages/react-core/src/components/Popover/examples/Popover.md Outdated
<Button>Toggle popover</Button>
</Popover>
</div>;
```ts isBeta file="./PopoverWithIconInTheTitle
Copy link
Contributor

@tompsota tompsota Aug 3, 2022

Choose a reason for hiding this comment

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

In addition to the previous comment, you are missing also file extension

packages/react-core/src/components/Popover/examples/PopoverAlert.tsx Outdated
};

return (
<div>
Copy link
Contributor

@tompsota tompsota Aug 3, 2022

Choose a reason for hiding this comment

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

You can use <React.Fragment> (or <>) instead of wrapper <div>

thatblindgeye
thatblindgeye approved these changes Aug 15, 2022
View reviewed changes
Copy link
Contributor

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

🎉

tompsota
tompsota suggested changes Aug 16, 2022
View reviewed changes
Copy link
Contributor

@tompsota tompsota left a comment

Choose a reason for hiding this comment

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

One last nit:

packages/react-core/src/components/Popover/examples/PopoverAdvanced.tsx Outdated
};

return (
<div>
Copy link
Contributor

@tompsota tompsota Aug 16, 2022

Choose a reason for hiding this comment

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

Suggested change
<div>
<>

packages/react-core/src/components/Popover/examples/PopoverAdvanced.tsx Outdated
<Button>Toggle popover</Button>
</Popover>
</div>
</div>
Copy link
Contributor

@tompsota tompsota Aug 16, 2022

Choose a reason for hiding this comment

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

Suggested change
</div>
</>

Copy link
Contributor Author

@dominik-petrik dominik-petrik Aug 16, 2022

Choose a reason for hiding this comment

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

On it!

tompsota
tompsota approved these changes Aug 16, 2022
View reviewed changes
Copy link
Contributor

@tompsota tompsota left a comment

Choose a reason for hiding this comment

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

Thanks!

tlabaj
tlabaj approved these changes Aug 18, 2022
View reviewed changes
Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

LGTM

@tlabaj tlabaj merged commit 0d3f7be into patternfly:main Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tlabaj tlabaj tlabaj approved these changes

@thatblindgeye thatblindgeye thatblindgeye approved these changes

+1 more reviewer

@tompsota tompsota tompsota approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Popover: convert examples to TypeScript

6 participants

@dominik-petrik @patternfly-build @nicolethoen @tompsota @tlabaj @thatblindgeye