Skip to content

Clear console on hot reload #544

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

Closed
wants to merge 3 commits into from
Closed

Conversation

ethanh1223
Copy link

@ethanh1223 ethanh1223 commented Jul 17, 2017

Adds #519

Clearing console in ComponentDidMount of Preview makes a lot more sense than Playground -- now understand how the two are related :)

Didn't think a doc update was necessary here.

Copy link
Member

@sapegin sapegin left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request!

@@ -49,6 +49,9 @@ export default class Preview extends Component {
};

componentDidMount() {
if (console.clear) { // eslint-disable-line no-console
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this condition?

Copy link

Choose a reason for hiding this comment

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

I believe it is because the tests fail with a console.clear is not a function if it isn't included.

Copy link
Member

Choose a reason for hiding this comment

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

Then it should be polyfilled in test/jestsetup.js, not hacked around in the main code.

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 also mock/spy on that function within the test itself, if you wanted:

global.console.clear = jest.fn();

Then you can assert that console.clear was called 😁

Copy link
Member

@sapegin sapegin left a comment

Choose a reason for hiding this comment

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

See the comments above.

@ndelangen
Copy link

Hey @ethanh1223 it looks like this PR kind of got forgotten, do you think it would be possible for you to finish this one?

It would definitely be awesome if you could. If not I'm sure @sapegin can take over to get this work merged!

@sapegin
Copy link
Member

sapegin commented Aug 29, 2017

@ndelangen What’s your interest here? ;-)

@ethanh1223 Let me know if you need any help. I would try what @tizmagik is suggesting. If you don’t have time to finish the PR soon — let us know, we’ll find someone else.

@sapegin
Copy link
Member

sapegin commented Oct 14, 2017

Finished in #638.

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.

5 participants