Skip to content

fix: apply resizing fix #47

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

Merged
merged 1 commit into from
Feb 24, 2025

Conversation

koko57
Copy link

@koko57 koko57 commented Feb 13, 2025

Details

Implement cache invalidation using ⁠react-window's ⁠resetAfterIndex API and combining it with another useLayoutEffect in this component - to prevent top value miscalculation for the PageRenderer.

Related Issues

Expensify/App#55941

Manual Tests

  1. In EApp (web/mWeb) go to an uploaded receipt.
  2. Open the preview.
  3. Verify that no blank space is rendered above the receipt.
  4. Repeat a few times - verify that the receipt is always rendered correctly.
  5. Try to scroll down and resize the screen - there should be no problems with resizing and the scroll position.
Screen.Recording.2025-02-13.at.13.48.45.mp4

Linked PRs

@koko57
Copy link
Author

koko57 commented Feb 13, 2025

cc @rezkiy37

Copy link
Contributor

@rezkiy37 rezkiy37 left a comment

Choose a reason for hiding this comment

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

Also, we need to update the example once it is deployed.

@@ -1,6 +1,6 @@
{
"name": "react-fast-pdf",
"version": "1.0.25",
"version": "1.0.26",
Copy link
Contributor

Choose a reason for hiding this comment

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

The bot bumps it automatically.

Suggested change
"version": "1.0.26",

Copy link
Author

Choose a reason for hiding this comment

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

strange, it should be already like that on main

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I see it. What's going on? 😅

Screenshot 2025-02-13 at 16 11 59 Screenshot 2025-02-13 at 16 11 55

Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind.

Copy link
Contributor

@dangrous dangrous left a comment

Choose a reason for hiding this comment

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

This looks good! What's the process for getting these out? Do I just merge? What do I need to do after? Thanks!

@@ -8,3 +8,4 @@ dist/

# NPM file created by GitHub actions
.npmrc
.idea/
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB - what's this change?

Copy link
Author

Choose a reason for hiding this comment

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

it's for Webstorm users - to ignore local files that Webstorm creates

@koko57
Copy link
Author

koko57 commented Feb 24, 2025

hello! just a friendly bump on this one 😃

@dangrous dangrous merged commit 5cab651 into Expensify:main Feb 24, 2025
2 checks passed
@os-botify
Copy link
Contributor

os-botify bot commented Feb 24, 2025

🚀 Published to npm in 1.0.27 🎉

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.

4 participants