Skip to content

Spinner changes #45

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Spinner changes #45

wants to merge 1 commit into from

Conversation

sumateja
Copy link

@sumateja sumateja commented Jul 4, 2020

Added Spinner code changes again by taking a new fork

@sumateja
Copy link
Author

sumateja commented Jul 4, 2020

Please approve my Pull request

Copy link
Contributor

@madhuni madhuni left a comment

Choose a reason for hiding this comment

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

I have provided a couple of comments. Also, your code is not properly formatted. Please run the prettier script before you push the commit to the remote branch.

}


const Spinner=(props)=>{
Copy link
Contributor

Choose a reason for hiding this comment

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

Please de-structure your props for better code readability. e.g.
const Spinner = ({type, children}) => {}

setMarkdown(text)
})

})
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we not adding anything in the dependencies array? Not providing that in useEffect will lead to serious issues of memory leaks and can cause the performance.

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.

2 participants