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

Update emoji font size if it was updated in markdownStyle #475

Merged
merged 4 commits into from
Sep 12, 2024

Conversation

VickyStash
Copy link
Contributor

Details

This PR fixes iOS issue, when emoji fontSize updates in the markdownStyle has no right-away effect on the emoji display.

Related Issues

GH_LINK

Manual Tests

  1. Update App.tsx example with the next updates:
// Specify default markdownStyle value
const [markdownStyle, setMarkdownStyle] = React.useState({emoji: {fontSize: 19}});

// Display markdownStyles to see how it's updated
<Text>Markdown style: {JSON.stringify(markdownStyle)}</Text>

...

// Add two buttons to update emoji font size
<Button
  title="Update emoji font size to 27"
  onPress={() =>
    setMarkdownStyle({
      emoji: {
        fontSize: 27,
      },
    })
  }
/>
<Button
  title="Update emoji font size to 19"
  onPress={() =>
    setMarkdownStyle({
      emoji: {
        fontSize: 19,
      },
    })
  }
/>
  1. Enter a couple of emojis and some text in the input
  2. Tap on buttons to update the emoji font size. See that the emoji font size updated in the input right away.
fixed_bug.mp4

Linked PRs

N/A

Copy link

github-actions bot commented Sep 9, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@VickyStash
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@VickyStash
Copy link
Contributor Author

recheck

@tomekzaw
Copy link
Collaborator

Reviewing the PR

@tomekzaw
Copy link
Collaborator

@VickyStash Could you please merge main?

@tomekzaw
Copy link
Collaborator

@VickyStash Also, it looks like the change has been extracted from #356, was that intentional? Do you think we should merge your PR first and then follow with #356 or we should merge just #356?

@VickyStash
Copy link
Contributor Author

@tomekzaw I'm okay to merge the #356 only and close this PR.
I've tried to confirm it the issue comment. I had some doubts why the #356 was open from May.

@tomekzaw
Copy link
Collaborator

@VickyStash Thanks for your response. So it looks like your PR along with changes from #474 indeed fixes the emoji font size issue on RN 0.75. However, there's still a problem when updating text font size:

Screen.Recording.2024-09-12.at.11.45.39.mov

If you're okay with closing this PR in favor of #356 then let's do that.

Thanks again for your investigation – Expensify/App#47547 (comment) is the best summary of what happened here.

@VickyStash VickyStash closed this Sep 12, 2024
@VickyStash
Copy link
Contributor Author

Closed in favor of #356

@tomekzaw
Copy link
Collaborator

Actually we've found one regression in #356 (comment) so we can't merge #356 in its current form.

Luckily, this regression doesn't happen on #475 (this PR):

Screen.Recording.2024-09-12.at.12.00.16.mov

So let's merge this PR as it fixes emoji.fontSize issue and does not break text suggestions.

@tomekzaw tomekzaw reopened this Sep 12, 2024
@tomekzaw tomekzaw merged commit 8ffc798 into Expensify:main Sep 12, 2024
7 of 9 checks passed
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.

2 participants