Skip to content
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

convert App/ directory to use TypeScript #3125

Merged
merged 4 commits into from
Jun 2, 2024
Merged

convert App/ directory to use TypeScript #3125

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6c01051
convert App/ directory to use TypeScript
mehrdadrafiee Nov 21, 2023
227252e
ignore lines one by one
mehrdadrafiee Nov 21, 2023
7e87136
Merge remote-tracking branch 'upstream/master'
mehrdadrafiee May 30, 2024
00f5314
addresses the comment and uses the builtin type
mehrdadrafiee May 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
144 changes: 84 additions & 60 deletions src/components/App/App.jsx → src/components/App/App.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// @ts-nocheck
Copy link
Member

Choose a reason for hiding this comment

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

can we just apply ts-ignore to specific lines instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

import darkBaseTheme from 'material-ui/styles/baseThemes/darkBaseTheme';
import getMuiTheme from 'material-ui/styles/getMuiTheme';
import MuiThemeProvider from 'material-ui/styles/MuiThemeProvider';
Expand All @@ -6,8 +7,7 @@ import Helmet from 'react-helmet';
import { connect } from 'react-redux';
import { Route, Switch, withRouter } from 'react-router-dom';
import styled from 'styled-components';

import Combos from './../Combos/Combos';
import Combos from '../Combos/Combos';
import Api from '../Api';
import Subscription from '../Subscription';
import constants from '../constants';
Expand All @@ -33,57 +33,78 @@ import GlobalStyle from './GlobalStyle';
import muiTheme from './muiTheme';
import config from '../../config';

const StyledDiv = styled.div`
interface AppStylesProps {
open?: boolean;
location: {
pathname?: string;
};
}

interface Back2TopStylesProps {
open?: boolean;
location: {
pathname: string;
};
}

interface AppProps {
location: { [key: string]: string }
strings: { [key: string]: string }
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can use GlobalStrings in types/common directory, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah if that's an alias

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm... I think will be facing the second option mentioned here: #3124 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

OK, in the meantime can you point all the string references to one place instead of duplicating { [key: string]: string }?

Copy link
Member

Choose a reason for hiding this comment

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

Actually it's probably best to use Record<string, string> which is a builtin type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 :)

}

const StyledDiv = styled.div<AppStylesProps>`
transition: ${constants.normalTransition};
position: relative;
display: flex;
flex-direction: column;
height: 100%;
left: ${(props) => (props.open ? '256px' : '0px')};
margin-top: 0px;

background-image: ${(props) =>
props.location.pathname === '/'
? 'url("/assets/images/home-background.png")'
? `url("/assets/images/home-background.png")`
: ''};
background-position: center top -56px;
background-repeat: ${(props) =>
props.location.pathname === '/' ? 'no-repeat' : ''};
`;

#back2Top {
position: fixed;
left: auto;
right: 0px;
top: auto;
bottom: 20px;
outline: none;
color: rgb(196, 196, 196);
text-align: center;
outline: none;
border: none;
background-color: rgba(0, 0, 0, 0.3);
width: 40px;
font-size: 14px;
border-radius: 2px;
cursor: pointer;
z-index: 999999;
opacity: 0;
display: block;
pointer-events: none;
-webkit-transform: translate3d(0, 0, 0);
padding: 3px;
transition: opacity 0.3s ease-in-out;

& #back2TopTxt {
font-size: 10px;
line-height: 12px;
text-align: center;
margin-bottom: 3px;
}
}

#back2Top:hover {
const Back2Top = styled.button<Back2TopStylesProps>`
position: fixed;
left: auto;
right: 0px;
top: auto;
bottom: 20px;
outline: none;
color: rgb(196, 196, 196);
text-align: center;
outline: none;
border: none;
background-color: rgba(0, 0, 0, 0.3);
width: 40px;
font-size: 14px;
border-radius: 2px;
cursor: pointer;
z-index: 999999;
opacity: 0;
display: block;
pointer-events: none;
-webkit-transform: translate3d(0, 0, 0);
padding: 3px;
transition: opacity 0.3s ease-in-out;

&:hover {
background-color: rgb(26, 108, 239);
}

#back2TopTxt {
font-size: 10px;
line-height: 12px;
text-align: center;
margin-bottom: 3px;
}
`;

const StyledBodyDiv = styled.div`
Expand All @@ -100,33 +121,36 @@ const AdBannerDiv = styled.div`
text-align: center;
margin-bottom: 5px;

& img {
img {
margin-top: 10px;
max-width: 100%;
}
`;

const App = (props) => {
const { strings, location } = props;
declare let window: Window & { adsbygoogle: any }

const back2Top = React.useRef();
const App = (props: AppProps) => {
const { strings, location } = props;
const back2Top = React.useRef<HTMLButtonElement>();

React.useEffect(() => {
const handleScroll = () => {
let wait = false;
const { current } = back2Top;
if (!wait) {

if (!wait && current) {
if (
document.body.scrollTop > 1000 ||
document.documentElement.scrollTop > 1000
) {
current.style.opacity = 1;
current.style.opacity = '1';
current.style.pointerEvents = 'auto';
} else {
current.style.opacity = 0;
current.style.opacity = '0';
current.style.pointerEvents = 'none';
}
}

setTimeout(() => {
wait = !wait;
}, 300);
Expand All @@ -137,7 +161,7 @@ const App = (props) => {
return () => {
window.removeEventListener('scroll', handleScroll);
};
});
}, []);
Copy link
Member

Choose a reason for hiding this comment

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

doesn't this change the behavior to only happen once?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

If they scroll back down though we want to reenable it right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


React.useLayoutEffect(() => {
window.scrollTo(0, 0);
Expand All @@ -152,7 +176,7 @@ const App = (props) => {
}, []);

return (
<MuiThemeProvider muiTheme={getMuiTheme(darkBaseTheme, muiTheme)}>
<MuiThemeProvider muiTheme={getMuiTheme(darkBaseTheme, muiTheme)}> {/* muiTheme types are missing here */}
<GlobalStyle />
<StyledDiv {...props}>
<Helmet
Expand All @@ -161,15 +185,15 @@ const App = (props) => {
/>
<Header location={location} />
<StyledBodyDiv {...props}>
<AdBannerDiv>
{includeAds && config.VITE_ENABLE_ADSENSE && (
<ins
className="adsbygoogle"
style={{display: 'block', width: 728, height: 90, margin: 'auto'}}
data-ad-client="ca-pub-5591574346816667"
data-ad-slot="9789745633"
/>)}
</AdBannerDiv>
<AdBannerDiv>
{includeAds && config.VITE_ENABLE_ADSENSE && (
<ins
className="adsbygoogle"
style={{ display: 'block', width: 728, height: 90, margin: 'auto' }}
data-ad-client="ca-pub-5591574346816667"
data-ad-slot="9789745633"
/>)}
</AdBannerDiv>
<Switch>
<Route exact path="/" component={Home} />
<Route exact path="/matches/:matchId?/:info?" component={Matches} />
Expand Down Expand Up @@ -214,15 +238,15 @@ const App = (props) => {
{includeAds && config.VITE_ENABLE_ADSENSE && (
<ins
className="adsbygoogle"
style={{display: 'block', width: 728, height: 90, margin: 'auto'}}
style={{ display: 'block', width: 728, height: 90, margin: 'auto' }}
data-ad-client="ca-pub-5591574346816667"
data-ad-slot="7739169508"
/>
)}
</AdBannerDiv>
<Footer />
<button
ref={back2Top}
<Back2Top
ref={back2Top} // Type 'undefined' is not assignable to type 'HTMLButtonElement | null'
id="back2Top"
title={strings.back2Top}
onClick={() => {
Expand All @@ -232,14 +256,14 @@ const App = (props) => {
>
<div>&#9650;</div>
<div id="back2TopTxt">{strings.back2Top}</div>
</button>
</Back2Top>
</StyledDiv>
</MuiThemeProvider>
);
};

const mapStateToProps = (state) => ({
const mapStateToProps = (state: any) => ({
strings: state.app.strings,
});

export default connect(mapStateToProps)(withRouter(App));
export default connect(mapStateToProps)(withRouter(App)); // Property 'strings' is missing in type
17 changes: 8 additions & 9 deletions src/components/App/AppLogo.jsx → src/components/App/AppLogo.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import React from 'react';
import { connect } from 'react-redux';
import PropTypes from 'prop-types';
import { Link } from 'react-router-dom';
import styled from 'styled-components';
import constants from '../constants';
Expand All @@ -16,21 +15,21 @@ const StyledLink = styled(Link)`
}
`;

const AppLogo = ({ size, strings, onClick }) => (
interface AppLogoProps {
size: string
strings: { [key: string]: string }
onClick: () => void
}

const AppLogo = ({ size, strings, onClick }: AppLogoProps) => (
<StyledLink aria-label="Go to the Open Dota homepage" to="/" onClick={onClick}>
<span style={{ fontSize: size }}>
{strings.app_name && `<${strings.app_name}/>`}
</span>
</StyledLink>
);

AppLogo.propTypes = {
size: PropTypes.string,
strings: PropTypes.shape({}),
onClick: PropTypes.func,
};

const mapStateToProps = state => ({
const mapStateToProps = (state: any) => ({
strings: state.app.strings,
});

Expand Down
0 src/components/App/index.js → src/components/App/index.ts
View file
Open in desktop
File renamed without changes.
0 src/components/App/muiTheme.js → src/components/App/muiTheme.ts
View file
Open in desktop
File renamed without changes.