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

πŸš€ Enhance Pull-to-Refresh Functionality with Native Sync and Improvements πŸ”„ #48632

Closed
wants to merge 11 commits into from

Conversation

krishpranav
Copy link

Summary:

✨ Enhancing the Pull-to-Refresh Functionality:
This pull request optimizes the refresh control behavior for both iOS and Android platforms. It resolves synchronization issues between the JS and native components, ensuring smoother and faster pull-to-refresh experiences. The update addresses UI feedback and performance improvements, making the feature more responsive and reliable. πŸš€

Changelog:

  • [ANDROID|IOS] [CHANGED] - Improved synchronization of the pull-to-refresh component between JS and native layers to ensure consistent behavior across platforms. πŸ”„
  • [GENERAL] [ADDED] - Enhanced UI feedback for the refresh indicator, improving user experience by providing a smoother and more responsive refresh action. 🎯
  • [GENERAL] [FIXED] - Resolved issues with refresh control behavior, ensuring it stays in sync with the refresh state across devices. πŸ› οΈ

Test Plan:

  • Ran the application on both iOS and Android devices to ensure pull-to-refresh functionality is working as expected. πŸ“±
  • Verified that the refresh indicator synchronizes correctly with the JS state, and the UI feedback is smooth and responsive. βœ…
  • Ensured that no regression issues occurred with other components while testing the refresh behavior. πŸ§ͺ

Issue Tagging:

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Jan 13, 2025
// $FlowFixMe[incompatible-type] asserted above
// $FlowFixMe[unsafe-addition]
const constants = NativeBlobModule.getConstants();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this moved?

I understand that $FlowFixMe directives were related to the line below?

Copy link
Author

Choose a reason for hiding this comment

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

resolved

@@ -76,7 +73,6 @@ export class URL {
// Do nothing.
}

// $FlowFixMe[missing-local-annot]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this removed?

Copy link
Author

Choose a reason for hiding this comment

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

resolved

@@ -104,51 +100,54 @@ export class URL {
}
this._url = `${baseUrl}${url}`;
}

// Parsing the URL to use for accessors
this._parsedUrl = new globalThis.URL(this._url);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why globalThis here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, can this be done lazily, to avoid parsing if the accessors are not ever used?

Copy link
Author

Choose a reason for hiding this comment

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

resolved

Copy link
Author

@krishpranav krishpranav left a comment

Choose a reason for hiding this comment

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

✨ What's New?

  • Lazy Parsing Optimization: Implemented a _parseUrlIfNeeded method to defer parsing of URLs until required. This reduces unnecessary overhead, improving performance for cases where URL accessors aren't used. πŸ•’πŸ’¨
  • Improved Compatibility: Enhanced fallback for globalThis by including support for window, ensuring seamless usage across diverse environments. πŸŒŽπŸ”—

πŸ’‘ Why It Matters?
These changes optimize the URL module for better efficiency and flexibility, especially in resource-constrained or varied runtime scenarios. By introducing lazy parsing, we're ensuring that operations are performed only when needed, aligning with performance best practices. βš‘πŸ› οΈ

πŸ“ Impact:

  • Boosts performance πŸš€
  • Ensures compatibility in broader environments πŸ–₯οΈπŸ“±

Thanks for reviewing! Let me know if there's anything else you'd like improved. πŸ™Œ

// $FlowFixMe[incompatible-type] asserted above
// $FlowFixMe[unsafe-addition]
const constants = NativeBlobModule.getConstants();
Copy link
Author

Choose a reason for hiding this comment

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

resolved

@@ -76,7 +73,6 @@ export class URL {
// Do nothing.
}

// $FlowFixMe[missing-local-annot]
Copy link
Author

Choose a reason for hiding this comment

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

resolved

@@ -104,51 +100,54 @@ export class URL {
}
this._url = `${baseUrl}${url}`;
}

// Parsing the URL to use for accessors
this._parsedUrl = new globalThis.URL(this._url);
Copy link
Author

Choose a reason for hiding this comment

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

resolved

@facebook-github-bot
Copy link
Contributor

@rshest has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@rshest
Copy link
Contributor

rshest commented Jan 14, 2025

@krishpranav The test_js and lint jobs on CI are broken.

Could you please fix those? Thanks.

Copy link
Contributor

@rshest rshest left a comment

Choose a reason for hiding this comment

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

Please do fix the broken test jobs on CI.

@rshest
Copy link
Contributor

rshest commented Jan 14, 2025

@krishpranav Another thing to note is that neither title, nor description of this PR has anything to do with what it actually does.

Can you elaborate here?

@javache
Copy link
Member

javache commented Jan 14, 2025

This PR's description does not match its content.

@javache javache closed this Jan 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants