convert App/ directory to use TypeScript #3125
Conversation
src/components/App/App.tsx
Outdated
| @@ -1,3 +1,4 @@ | |||
| // @ts-nocheck | |||
There was a problem hiding this comment.
can we just apply ts-ignore to specific lines instead
src/components/App/App.tsx
Outdated
|
|
||
| interface AppProps { | ||
| location: { [key: string]: string } | ||
| strings: { [key: string]: string } |
There was a problem hiding this comment.
Can we use one shared type for strings so if we figure out how to type it better later we only have one spot to update?
There was a problem hiding this comment.
can use GlobalStrings in types/common directory, right?
There was a problem hiding this comment.
hmmm... I think will be facing the second option mentioned here: #3124 (comment)
There was a problem hiding this comment.
OK, in the meantime can you point all the string references to one place instead of duplicating { [key: string]: string }?
There was a problem hiding this comment.
Actually it's probably best to use Record<string, string> which is a builtin type
There was a problem hiding this comment.
I just updated the PR with the builtin type as you suggested. I can go ahead and update other instances of { [key: string]: string } in a separate PR if you approve.
thanks and sorry for late reply :)
src/components/App/App.tsx
Outdated
| @@ -137,7 +161,7 @@ const App = (props) => { | |||
| return () => { | |||
| window.removeEventListener('scroll', handleScroll); | |||
| }; | |||
| }); | |||
| }, []); | |||
There was a problem hiding this comment.
doesn't this change the behavior to only happen once?
There was a problem hiding this comment.
isn't it the expected behavior? clicking on the "back to top" button takes the user to the top of the screen and then the button disappears. user won't be able to click it multiple times.
There was a problem hiding this comment.
If they scroll back down though we want to reenable it right?
There was a problem hiding this comment.
yeah that is what's happening right now. the button appears every time user scrolls 1000px from top and can be clicked to take the user to the top of the screen. to be honest I think it's best practice to have [] in the useEffect hook and if anything it should have a dependency on the scroll.
Please review.
As part of refactoring, this PR converts
src/components/App/directory to use TypeScript. There were some spots that I wasn't sure how to handle the types so I had to ignore them (@ts-nocheck) for now but feel free to pull down the changes and update the PR.Since the styled-component is a bit old, I wasn't able to convert the
src/components/App/GlobalStyles.jsxto TypeScript. Can be done once we upgrade to a newer version.