-
Notifications
You must be signed in to change notification settings - Fork 377
chore: Convert examples to typescript #6791
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
| import React from 'react'; | ||
| import { Brand } from '@patternfly/react-core'; | ||
|
|
||
| export const BrandBasic: React.FunctionComponent = () => <Brand src="./pfLogo.svg" alt="Patternfly Logo" />; |
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.
did it not work if you tried to import the svg? might be worth opening an issue if we have an issue with svg imports.
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.
We don't support importing svg files at the moment (probably a tsconfig setting, possibly webpack config related), but importing the SVG didn't seem correct to begin with because src takes a string type (implying a path to a source, not the source itself), not an SVGElement or HTMLElement type.
I don't perceive it as an issue unless we need to import svg's somewhere. In this case we don't. Then there's the question of, do we really want to treat them as their own modules or do we just create function components that return JSX interpretations of the svg, which is another approach which wouldn't require any tsconfig/webpack changes.
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.
Looking into this further, the default import of an SVG element returns a string, so a module file was added to accommodate for typescript not recognizing SVG imports as modules (index.d.ts), and this example was reverted back to using that imported pfLogo.
ea44b6a to
e287bfc
Compare
tlabaj
left a comment
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.
Looks great! Just one little nit.
|
|
||
| export const BannerBasic: React.FunctionComponent = () => ( | ||
| <Flex direction={{ default: 'column' }}> | ||
| <Banner>Default banner</Banner> |
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.
Usually our examples match the core examples. For that reason, I would add back the <br />
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.
Done. And updated other recent instances where I was using flexbox instead of breaks and empty spaces as well.
e287bfc to
ec9ca43
Compare
tlabaj
left a comment
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.
Looks great!
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Closes: