chore(Page): convert examples to TypeScript #7735
Conversation
|
Preview: https://patternfly-react-pr-7735.surge.sh A11y report: https://patternfly-react-pr-7735-a11y.surge.sh |
tompsota
left a comment
tompsota
left a comment
There was a problem hiding this comment.
Hello @dominik-petrik, great job with the conversion! 🎉
I have just a couple of minor comments.
| ); | ||
| } | ||
| } | ||
| ```ts file="./VerticalNav.tsx" |
There was a problem hiding this comment.
Usually the file names are prefixed with the component name, in this case Page. Could you update this (and do the same with all other files as well)?
| ```ts file="./VerticalNav.tsx" | |
| ```ts file="./PageVerticalNav.tsx" |
|
|
||
| ### Vertical nav using PageHeader component | ||
|
|
||
| This example is provided becuase PageHeader and PageHeaderTools are still in use; however, going forward Masthead and Toolbar should be used to make headers rather than PageHeader and PageHeaderTools. |
There was a problem hiding this comment.
Could you please fix also this typo?
becuase -> because
| } from '@patternfly/react-core'; | ||
| import BarsIcon from '@patternfly/react-icons/dist/esm/icons/bars-icon'; | ||
|
|
||
| export const VerticalPage: React.FunctionComponent = () => { |
There was a problem hiding this comment.
Component name should correspond with the file name. Also please check other files.
| export const VerticalPage: React.FunctionComponent = () => { | |
| export const PageMainSectionPadding: React.FunctionComponent = () => { |
| </Toolbar> | ||
| ); | ||
|
|
||
| const Header = ( |
There was a problem hiding this comment.
I think that the convention is to use camelCase, is that correct @nicolethoen?
| const Header = ( | |
| const header = ( |
There was a problem hiding this comment.
That's correct - we generally use camelCase for our variable names and PascalCase for component names. Then any text, whether prop description or example titles are all Sentence case.
| <MastheadContent>{headerToolbar}</MastheadContent> | ||
| </Masthead> | ||
| ); | ||
| const Sidebar = <PageSidebar nav="Navigation" isNavOpen={isNavOpen} id="main-padding-sidebar" />; |
There was a problem hiding this comment.
I'd add newlines around const declarations (where missing) to make the code easier to read. However I don't have strict opinion on this, what do you think @nicolethoen?
| const Sidebar = <PageSidebar nav="Navigation" isNavOpen={isNavOpen} id="main-padding-sidebar" />; | |
| const Sidebar = <PageSidebar nav="Navigation" isNavOpen={isNavOpen} id="main-padding-sidebar" />; | |
There was a problem hiding this comment.
I don't have a particularly strong opinion here. I think there are definitely times that using line breaks to clearly group related concepts or separate distinct concepts/steps can help with readability. I think in this case, @dominik-petrik can make a judgement call if he thinks adding the space is an improvement.
| ); | ||
| const Sidebar = <PageSidebar nav="Navigation" isNavOpen={isNavOpen} id="main-padding-sidebar" />; | ||
| return ( | ||
| <Page header={Header} sidebar={Sidebar}> |
There was a problem hiding this comment.
The original example actually didn't have a sidebar passed in. Maybe we should keep it that way?
There was a problem hiding this comment.
We discussed this via slack and we agreed on maybe it being an oversight so I added the sidebar. But no problem for me to fix it. 😊
There was a problem hiding this comment.
nah - sounds good for me! :)
|
I have hopefully resolved all required changes. Let me know if I missed something or if there is anything else that needs to be done. |
|
@dominik-petrik it looks good, it just needs a rebase! :) |
5 participants
What: Closes #7711