Conversation
|
@a-x- can you help us review this? |
| debug('init printable', this.props.name); | ||
| const isSingle = (this.props.main || this.props.single); | ||
| this.context.printProvider && this.context.printProvider.regPrintable(this.props.name, <Print {...this.props} />, isSingle); | ||
| useEffect(() => { |
There was a problem hiding this comment.
looks like this useEffect should be componentDidMount equivalent, because, e.g. window.matchMedia('print').onchange must be called at once,
so, as I understand, we need add empty list of dependencies ([]) here: useEffect(() => {...}, []);
There was a problem hiding this comment.
also, we need to verify all the useEffects' dependencies
sibelius
left a comment
sibelius
left a comment
There was a problem hiding this comment.
I don’t think this influence the react devtool
|
I'll check the comments and suggestions and work on the changes later |
This comment has been minimized.
This comment has been minimized.
|
@a-x- any progress on this? anything we can do to help? |
|
@a-x- Hey, anything I can do to help to get this merged? Just let me know :) |
|
can we merge and release a new version? it would be cool to add some react-testing-library tests after this |
3 participants
Closes #43