Skip to content

Conversation

@maksymilianrojek
Copy link
Collaborator

Summary

Changes:

  • added types to web implementation
  • extracted common types to /common/types.ts file

Test Plan

  • both example and example-web works correctly (no lint or typecheck issues)

Compatibility

OS Implemented
iOS
Android
Web

This PR will be merged to a collective PR where all web-related changes will be kept until the implementation is complete.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR enhances type safety by adding comprehensive TypeScript types to the web implementation and extracting common types shared between native and web platforms into a centralized location.

Key changes:

  • Created src/common/types.ts to house shared type definitions (OnChangeTextEvent, OnChangeHtmlEvent, OnChangeStateEvent, OnMentionDetected, OnChangeMentionEvent, and EnrichedTextInputInstanceBase)
  • Added complete type definitions to src/web/EnrichedTextInput.tsx including props, events, and imperative handle methods
  • Fixed typo in function name from "Missconfigured" to "Misconfigured" in native implementation

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/common/types.ts New file containing shared type definitions extracted from native implementation
src/native/EnrichedTextInputNativeComponent.ts Updated to import shared types from common/types instead of defining them locally
src/native/EnrichedTextInput.tsx Updated to import shared types from common/types and fixed typo in function name
src/web/EnrichedTextInput.tsx Transformed from minimal stub to fully-typed implementation with complete type definitions and imperative handle methods
src/index.tsx Added exports for common types to make them available to web consumers
src/index.native.tsx Reorganized type exports to import common types from centralized location
apps/example-web/src/App.tsx Updated to pass placeholder prop instead of hardcoding it in component

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 6 to 9
OnLinkDetected,
OnMentionDetected,
OnChangeSelectionEvent,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason why we essentially omit these two from the web editor? I think we're trying to achieve API parity, even if we use no-ops in some cases (which might be true for OnChangeSelectionEvent on web), I think we still want the API interface to be identical.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On the web those two are in './web/EnrichedTextInput' file, so they are exported using export * from './web/EnrichedTextInput';. I see it now that it might be confusing

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, now I see native side is using codegen types in these. I wonder if the users will run into typing issues if they had a shared codebase for both web and native and there would be essentially two possible definitions for these interfaces..

Copy link
Collaborator

Choose a reason for hiding this comment

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

@maksymilianrojek any resolutions to that one? We can definitely proceed with the PR once this matter is dealt with.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've added common types to the types.ts, added handlers to the native side to "translate" native events to the common event type. c307304

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