chore(Nav): convert demos to TS#9487
Conversation
|
Preview: https://patternfly-react-pr-9487.surge.sh A11y report: https://patternfly-react-pr-9487-a11y.surge.sh |
packages/react-core/src/demos/examples/Nav/NavHorizontalWithSubnav.tsx
Outdated
Show resolved
Hide resolved
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
One other comment below. Also, in your attempt to resolve the ts file=... issue, were you running into errors only when trying to create a d.ts file, or even when updating the Dashboard___ files to tsx as well?
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
Depending on whether we can get the Dashboard components updated to typescript without issue, it may make sense to try getting those in first just to avoid having to make another update for demos to update the js= to ts=.
jenny-s51
left a comment
jenny-s51
left a comment
There was a problem hiding this comment.
Nice work on this @adamviktora - these conversions are looking great! Left a comment below
| </PageSection> | ||
| <PageSection> | ||
| <Gallery hasGutter> | ||
| {Array.apply(0, Array(10)).map((_x: any, i: React.Key | null | undefined) => ( |
There was a problem hiding this comment.
For readability, should we be more specific with these parameter names? Also wondering if the null | undefined types can be removed here
There was a problem hiding this comment.
Yeah, good idea, I changed _x to _value and i to index. And you are right, the null and undefined were unnecessary.
There was a problem hiding this comment.
Awesome, looks great @adamviktora ! Can we apply the same change to all remaining instances of Array.apply?
There was a problem hiding this comment.
@jenny-s51 Done (at least in the Nav demos). Should I apply the change to all instances in the codebase?
There was a problem hiding this comment.
Looking good! For consistency that would be a good idea, but I'd say that is technically out of scope since it would involve updating other demos.
Could you file a standalone "tech debt" issue for that so we can merge this one in @adamviktora ?
warnings should disappear once issue patternfly#9544 is closed
jenny-s51
left a comment
jenny-s51
left a comment
There was a problem hiding this comment.
This is looking great following the recent updates to the DashboardHeader/DashboardWrapper imports @adamviktora !
Left one more comment in the existing thread above - should be good to merge once that's fixed 👍
4 participants
What: Closes #9376
I kept the
jsshortcuts instead oftsin theNav.mdfile like this:Because if I type
ts, I get this "cannot find a module" warning.This would require converting
DashboardHeader.jsandDashboardWrapper.jsto TS, or at least creating a.d.tsfile which would declare a module. I tried doing that but it lead to many changes through several files and still was getting some errors.On the website, there still stays TS icon, although
jsis used.