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.

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

feat: added snow #71

Merged
merged 13 commits into from
Dec 23, 2019
Merged

feat: added snow #71

merged 13 commits into from
Dec 23, 2019

Conversation

vladimir-sviridenko
Copy link
Contributor

No description provided.

@vladimir-sviridenko
Copy link
Contributor Author

@DrJekill352 DrJekill352 self-requested a review December 15, 2019 10:17
@mkinitcpio mkinitcpio self-requested a review December 15, 2019 19:20
startSnowflakes();
});

const getCanvasElementById = (id: string): HTMLCanvasElement => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

useCallback hook for function creation

return canvas;
};

const getCanvasRenderingContext2D = (canvas: HTMLCanvasElement): CanvasRenderingContext2D => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

useCallback hook for function creation

return context;
};

function startSnowflakes() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

useCallback hook for function creation

const requestId = React.useRef(0);

useEffect(() => {
startSnowflakes();
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can create useSnowflakes hook and incapsulate all logic inside


const MAIN_YELLOW = '#F2E14C';

const useStyles = makeStyles({
Copy link
Collaborator

Choose a reason for hiding this comment

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

please create useStyles variable outside component, otherwise this variable will be create on each component Render


return (
<>
<div className='snow-switcher'>
Copy link
Collaborator

Choose a reason for hiding this comment

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

please use styles that you created by makeStyles, the same for all className

@@ -0,0 +1,18 @@
#snow {
Copy link
Collaborator

Choose a reason for hiding this comment

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

please relocate styles to makeStyles function

@@ -0,0 +1,31 @@
@media screen and (min-width: 960px) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can add media by css-in-js, please relocate it inside makeStyles function,
if you need an example, you can find code with 'theme.breakpoints.'

@DrJekill352 DrJekill352 merged commit 2791b17 into js-machine:master Dec 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants