-
Notifications
You must be signed in to change notification settings - Fork 474
Bump React to version 18. #232
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
Conversation
c919520
to
8e197d2
Compare
This currently works as-is in my team's app, though there are warnings that do appear. For example:
This is related to rendering the highlights in PdfHighlighter.tsx:325. Since each page has its own highlight layer we need to find a "root" that can be used to render them. |
I don't know if asking for an update via a comment on this PR is appropriate, but I would like to know if there is an ETA for this fix since it has impacted our usage of this excellent library. Thank you! |
Thank you for your code. |
@keunbit the only way to fix the warning is to re-implement how the highlights are attached to the DOM. I haven't had the time to look into this. Thankfully it's just a warning and doesn't impact the library's functionality. |
…id creating new React apps when the relevant states & props change.
@keunbit that issue has been resolved with the latest version of this branch. Feel free to try it out! |
@joebutler2 cool work, thanks! would you say that this is ready for merging now? |
ReactDOM.render(<App />, document.getElementById("root")); | ||
const container = document.getElementById("root"); | ||
const root = createRoot(container!); | ||
root.render(<App tab="home" />); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This prop seems unnecessary.
pageNumber: string; | ||
scrolledToHighlightId: string; | ||
highlightTransform: ( | ||
highlight: any, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please fix any
types here?
@joebutler2 - thanks for your work. We are very interested in getting this PR merged into the release branch. Thank you! |
@ayush1852017 What |
But right now I have decided to go with React 17 only. The reason is, I am facing so many problems with AreaHighlights. |
For my project, not losing the highlights when resizing the browser window is a must-have. However, when using ayush1852017's fix (thank you!), the
|
I'm going to use this plugin for my new project, but in npm it's still doesn't work with react 18? because I'm getting warning about ReactDOM.render and makes text highlighting and scrolling doesn't work. Is there any chance the updated version is available in npm? Thank you |
@mochirfan Here is a link to the package containing this PR's code: https://github.com/orgs/credolabs/packages/npm/package/react-pdf-highlighter You'll have to download it through the Github package registry instead of NPM, but it might be a good solution in the mean time. |
Thanks everyone for the hard work and constructive comments on the PR! Wrote a short snippet about how to get up and running with the package using the GH package registry as described above - hope it helps in some way till the PR is merged |
Thank you! Been struggling with this too. You guys are great ❤️ |
Thanks @joebutler2
|
@k3ntar0 Hopefully this will give you a few leads - I'm not using yarn at all for my project so the .npmrc file I was using looked like this:
From there, installing the package via the command line or package.json should work. |
Thanks everyone, this is now published in version |
I think there is a bug though :-) If I scroll far enough so that PDF.js removes some pages and come back to the first page, I don't see highlights. Update: hopefully fixed with c78c07a (released in |
No description provided.