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: LEAP-828: Add handling for carriage return in domManager #5615

Merged
merged 11 commits into from
Apr 3, 2024

Conversation

Gondragos
Copy link
Collaborator

@Gondragos Gondragos commented Mar 21, 2024

  1. Update the dom Managed to incorporate the handling of carriage return (CR) characters. This is a necessary change as some operating systems or browsers use both line feed (LF) and carriage return characters.

  2. Previously we had a problem with missing regions when we are trying to render annotation from results that have a wrong order. It was caused by the problem with detecting area type (it thought it was an classification) when we have excessive fields in result (like a text). So in this commit I tried to fix it by adding expected values from value after creating an area and directly to the text field of RichTextRegion. (see HumanSignal/label-studio-frontend@6ef0657#diff-9386d502ec5e5d9e8b50c2c679499430ca58728900aac1a43de33d13ee2648f4)

PR fulfills these requirements

  • Tests for the changes have been added/updated
  • Docs have been added/updated
  • Best efforts were made to ensure docs/code are concise and coherent (checked for spelling/grammatical errors, commented out code, debug logs etc.)
  • Self-reviewed and ran all changes on a local instance

Describe the reason for change

There was a situation when, when opening an annotation with richtext, some regions could receive incorrect text values, which led to saving the results without text inside.

This change affects (describe how if yes)

  • Performance
  • Security
  • UX

Does this PR introduce a breaking change?

  • Yes, and covered entirely by feature flag(s)
  • Yes, and covered partially by feature flag(s)
  • No
  • Not sure (briefly explain the situation below)

What level of testing was included in the change?

  • e2e (codecept)
  • integration (cypress)
  • unit (jest)

Which logical domain(s) does this change affect?

HyperText, RichText, Text, domManager

@github-actions github-actions bot added the fix label Mar 21, 2024
Copy link

netlify bot commented Mar 21, 2024

Deploy Preview for label-studio-docs-new-theme canceled.

Name Link
🔨 Latest commit 5577733
🔍 Latest deploy log https://app.netlify.com/sites/label-studio-docs-new-theme/deploys/660d45f2967abc0008c03a8b

Copy link

netlify bot commented Mar 21, 2024

Deploy Preview for heartex-docs canceled.

Name Link
🔨 Latest commit 5577733
🔍 Latest deploy log https://app.netlify.com/sites/heartex-docs/deploys/660d45f26d05f2000812545e

Copy link
Collaborator

@hlomzik hlomzik left a comment

Choose a reason for hiding this comment

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

This PR definitely requires more description + it has a dangerous change in behaviour

Copy link
Collaborator

@hlomzik hlomzik left a comment

Choose a reason for hiding this comment

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

small corrections, but that's great fix!

Co-authored-by: hlomzik <hlomzik@gmail.com>
@hlomzik
Copy link
Collaborator

hlomzik commented Apr 3, 2024

/git merge

Workflow run
Successfully merged: 87 files changed, 11989 insertions(+), 8887 deletions(-)

@Gondragos Gondragos merged commit 23a1603 into develop Apr 3, 2024
29 checks passed
@Gondragos Gondragos deleted the fb-leap-828/empty-text-fields branch April 3, 2024 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants