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

Add onContentSizeChange prop #290

Merged
merged 3 commits into from
Apr 12, 2024
Merged

Conversation

Skalakid
Copy link
Collaborator

Details

This PR add onContentSizeChange prop to live markdown web implementation. This is a part of a jumping composer issue fix

Related Issues

GH_LINK

Manual Tests

  1. Add onContentSizeChange prop function
  2. Console log its event. Verify if height and width are correct

Linked PRs

Expensify/App#39267

@Skalakid Skalakid requested a review from tomekzaw April 11, 2024 15:27
Copy link
Collaborator

@tomekzaw tomekzaw left a comment

Choose a reason for hiding this comment

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

Looks okay to me, left some comments.

src/MarkdownTextInput.web.tsx Outdated Show resolved Hide resolved
Comment on lines 314 to 315
const newHeight = hostNode.offsetHeight;
const newWidth = hostNode.offsetWidth;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just wondering if there's any reason for this order (height before width), is this intended?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is no reason behind this, is it like better to put width first? Does it make any difference?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nope, just wanted to ask.

BartoszGrajdek
BartoszGrajdek previously approved these changes Apr 12, 2024
Copy link
Collaborator

@BartoszGrajdek BartoszGrajdek left a comment

Choose a reason for hiding this comment

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

LGTM! Just answer Tomek's questions

Copy link
Collaborator

@tomekzaw tomekzaw left a comment

Choose a reason for hiding this comment

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

LGTM

@Skalakid Skalakid merged commit c8948f8 into main Apr 12, 2024
4 checks passed
@Skalakid Skalakid deleted the @Skalakid/add-onContentSizeChange-prop branch April 12, 2024 08:57
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.

3 participants