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

impl(lazy-initialize): set content for handlePrint() and handleClick() via second optional argument using [content] #679

Merged
merged 14 commits into from
Feb 9, 2024

Conversation

isocroft
Copy link
Contributor

@isocroft isocroft commented Jan 4, 2024

Fixes #652

@MatthewHerbst
Copy link
Owner

Thanks! Will take a look at this later this evening

@isocroft
Copy link
Contributor Author

Hello @MatthewHerbst , i was hoping to get some feedback on this as soon as you can be available say this weekend ?

@MatthewHerbst
Copy link
Owner

Hi, apologies, have been slammed with work. Promise to review this within the next 48 hours

Copy link
Owner

@MatthewHerbst MatthewHerbst left a comment

Choose a reason for hiding this comment

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

Looks good, just curious if there's a way to simplify the logic at all maybe?

src/index.tsx Outdated Show resolved Hide resolved
src/index.tsx Outdated Show resolved Hide resolved
src/index.tsx Outdated Show resolved Hide resolved
src/index.tsx Outdated Show resolved Hide resolved
src/index.tsx Outdated Show resolved Hide resolved
@isocroft
Copy link
Contributor Author

Hello. @MatthewHerbst , I am done with the change requests you mandated based on your review.

Thanks a lot 🙏🏾

Copy link
Owner

@MatthewHerbst MatthewHerbst left a comment

Choose a reason for hiding this comment

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

Looks great! One last request: can we make the example much, much shorter? The examples for the other uses are about 25 lines each, and don't reference any random external dependencies. Can we make the added example similarly short, and remove any reference to external dependencies that most readers of the README will not be familiar with?

@isocroft
Copy link
Contributor Author

Ok @MatthewHerbst , I will do just that.

Thanks

@isocroft
Copy link
Contributor Author

isocroft commented Feb 3, 2024

All done now @MatthewHerbst 👍🏾

@isocroft isocroft changed the title impl(lazy-initialize): set content for handlePrint() and handleClick() via second argument using [lazyOption] impl(lazy-initialize): set content for handlePrint() and handleClick() via second optional argument using [content] Feb 3, 2024
Copy link
Owner

@MatthewHerbst MatthewHerbst left a comment

Choose a reason for hiding this comment

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

Ah, realized one last thing! Otherwise looks fantastic, and thank you so much for adding the examples!

src/index.tsx Outdated Show resolved Hide resolved
Copy link
Owner

@MatthewHerbst MatthewHerbst left a comment

Choose a reason for hiding this comment

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

Nice! Happy with this! Two small nits before we merge

src/index.tsx Outdated Show resolved Hide resolved
src/index.tsx Outdated Show resolved Hide resolved
@isocroft
Copy link
Contributor Author

isocroft commented Feb 8, 2024

Hello @MatthewHerbst , all done here

@MatthewHerbst MatthewHerbst merged commit 3cc32aa into MatthewHerbst:master Feb 9, 2024
@isocroft isocroft deleted the feature_lazy-initialize branch February 9, 2024 10:17
@MatthewHerbst
Copy link
Owner

Published in v2.15.0!

@Bowbaq
Copy link

Bowbaq commented Feb 14, 2024

This seems to break existing code by adding this new mandatory event argument, which doesn't appear to be used for anything.

> tsc && vite build

src/components/print-table/index.tsx:23:5 - error TS2554: Expected 1-2 arguments, but got 0.

23     handlePrint();
       ~~~~~~~~~~~~~

  node_modules/react-to-print/lib/hooks/useReactToPrint.d.ts:3:35
    3 type UseReactToPrintHookReturn = (event: unknown, content?: (() => React.ReactInstance | null)) => void;
                                        ~~~~~~~~~~~~~~
    An argument for 'event' was not provided.


Found 1 error in src/components/print-table/index.tsx:23

@MatthewHerbst
Copy link
Owner

This seems to break existing code by adding this new mandatory event argument, which doesn't appear to be used for anything.

> tsc && vite build

src/components/print-table/index.tsx:23:5 - error TS2554: Expected 1-2 arguments, but got 0.

23     handlePrint();
       ~~~~~~~~~~~~~

  node_modules/react-to-print/lib/hooks/useReactToPrint.d.ts:3:35
    3 type UseReactToPrintHookReturn = (event: unknown, content?: (() => React.ReactInstance | null)) => void;
                                        ~~~~~~~~~~~~~~
    An argument for 'event' was not provided.


Found 1 error in src/components/print-table/index.tsx:23

Ah crap, you're totally right, this should be optional. Will push out a fix now

@MatthewHerbst
Copy link
Owner

@Bowbaq fix published in v2.15.1, please let me know if you run into any additional issues, and thank you so much for the report!

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.

[ENHANCEMENT]: Set content function lazily to the handlePrint function returned by the useReactToPrint() hook
3 participants