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

Create method onBeforeGetContent #158

Closed
wants to merge 13 commits into from
Closed

Create method onBeforeGetContent #158

wants to merge 13 commits into from

Conversation

andfs
Copy link
Contributor

@andfs andfs commented Aug 8, 2019

Sometimes is useful to update the component to print before printing, for example, when someone needs to hide somethings in the printing.
The component triggers the method getContent an then calls onBeforePrinting, but at this time, the component is mounted and everything that you do in onBeforePrinting is not reflected in the component.
So with onBeforeGetContent is possible to update the component before printing

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.

Hi, thanks for the PR. A couple questions about the code. I like the idea, though we need to flesh it out a bit with how it would work.

Can you please update the docs as part of the PR, and, please provide an example of how this would work?

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

Also, we do not use Yarn for this package, so please remove the yarn.lock file

@andfs
Copy link
Contributor Author

andfs commented Aug 8, 2019

Hi, thanks for the PR. A couple questions about the code. I like the idea, though we need to flesh it out a bit with how it would work.

Can you please update the docs as part of the PR, and, please provide an example of how this would work?

Hi @MatthewHerbst . I'll do that :)

@MatthewHerbst
Copy link
Owner

Please don't change the formatting of the entire file. Besides the fact that we have lint rules which will break, it makes the diff impossible to read

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
example/ComponentToPrint/index.tsx Outdated Show resolved Hide resolved
example/index.tsx Outdated Show resolved Hide resolved
example/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
.vscode/launch.json Outdated Show resolved Hide resolved
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.

We're really close here, thanks for making all the changes, just a few small ones left.

src/index.tsx Outdated Show resolved Hide resolved
src/index.tsx Outdated Show resolved Hide resolved
src/index.tsx Outdated Show resolved Hide resolved
example/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.

Can you undo the formatting changes of the whole file please? Looking forward to getting this merged this week :)

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! Can you please just squash your commits now into a single commit and then I'll merge?

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.

🎉 awesome, thank you! Will merge now. I'm going to make another commit cleaning up a few things across the library. Hope to have a release with this included in it tonight or tomorrow!

@MatthewHerbst
Copy link
Owner

Oh wait, one thing, please squash the commits first! Do you know how to do that?

@andfs
Copy link
Contributor Author

andfs commented Aug 15, 2019

Oh wait, one thing, please squash the commits first! Do you know how to do that?

I think I squashed in my last push, didn't I?

@MatthewHerbst
Copy link
Owner

Oh wait, one thing, please squash the commits first! Do you know how to do that?

I think I squashed in my last push, didn't I?

Nope - there should only be a single commit after you squash, right now there are 13 commits.

@andfs
Copy link
Contributor Author

andfs commented Aug 15, 2019

Oh wait, one thing, please squash the commits first! Do you know how to do that?

I think I squashed in my last push, didn't I?

Nope - there should only be a single commit after you squash, right now there are 13 commits.

Sorry. I think I did it right.

I thought that was that:
74d61df

Could you tell me how to do it?

@MatthewHerbst
Copy link
Owner

Let me know if you have any trouble from this: https://github.com/wprig/wprig/wiki/How-to-squash-commits

@andfs
Copy link
Contributor Author

andfs commented Aug 15, 2019

Let me know if you have any trouble from this: https://github.com/wprig/wprig/wiki/How-to-squash-commits

sorry, but I wasn't able to make it work :/

@andfs
Copy link
Contributor Author

andfs commented Aug 17, 2019

Let me know if you have any trouble from this: https://github.com/wprig/wprig/wiki/How-to-squash-commits

There is another way to squash it?

@MatthewHerbst
Copy link
Owner

Can you add me to have permissions to push to your fork of react-to-print? I can fix the branch for you

@andfs
Copy link
Contributor Author

andfs commented Aug 18, 2019

Can you add me to have permissions to push to your fork of react-to-print? I can fix the branch for you

sure

@MatthewHerbst
Copy link
Owner

I'm trying to push it up to your branch but it's not letting me. I can keep trying, or I can push it as a new PR and change the author on the commit to you. Would that be ok with you?

@andfs
Copy link
Contributor Author

andfs commented Aug 19, 2019

I'm trying to push it up to your branch but it's not letting me. I can keep trying, or I can push it as a new PR and change the author on the commit to you. Would that be ok with you?

of course, no problem :)

@MatthewHerbst
Copy link
Owner

Squashed and ready to go in #160

@MatthewHerbst MatthewHerbst deleted the onBeforeGetContent branch August 20, 2019 20:00
@MatthewHerbst
Copy link
Owner

Published as v2.4.0. Thanks so much for your work on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants