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

Fix showing wrongly formatted text #374

Merged

Conversation

zfurtak
Copy link
Contributor

@zfurtak zfurtak commented Jun 7, 2024

Details

When typing text with command + b or command + i or command + u effect was applied to inputted text, which was unwanted.
Also connected issue: when text was pasted to an empty filed, it was in the colour of placeholder.
In these cases instead of <span> object with text, <b>, <i>, <u>.... was being created.

My proposal is to change the created object when it's not <span>

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.

I think it may be easier to just change the condition in src/web/parserUtils.ts (L206) to check if the firstChild of our input has the class root. If not it should regenerate the input content, meaning creating the span & parsing the content with ExpensiMark - this will remove any additional styles.

if (!rootSpan || !rootSpan.classList.contains('root') || rootSpan.innerHTML !== dom.innerHTML)

@zfurtak
Copy link
Contributor Author

zfurtak commented Jun 25, 2024

@BartoszGrajdek I agree this solution is much more clear but it leaves unwanted behaviour connected to placeholder.
Look below how it looks in the example app and Expensify App.
Or maybe we don't want to target this issue here?

Screen.Recording.2024-06-25.at.12.55.50.mov
Screen.Recording.2024-06-25.at.12.56.20.mov

Copy link
Collaborator

@Skalakid Skalakid left a comment

Choose a reason for hiding this comment

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

LGTM

@BartoszGrajdek
Copy link
Collaborator

We'll merge it after a version bump in E/App. To make sure in case of any regressions we don't have to add testing steps for this PR as well

@BartoszGrajdek BartoszGrajdek merged commit 8117369 into Expensify:main Jul 18, 2024
5 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.

3 participants