Skip to content

SF-2481 Fix verse number order in RTL editor when text has numbers - #4089

Merged
Nateowami merged 1 commit into
masterfrom
fix/SF-2481-rtl-bidi
Sep 9, 2026
Merged

SF-2481 Fix verse number order in RTL editor when text has numbers#4089
Nateowami merged 1 commit into
masterfrom
fix/SF-2481-rtl-bidi

Conversation

@Nateowami

@Nateowami Nateowami commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Before

Screenshot from 2026-09-08 16-04-36

After

Screenshot from 2026-09-08 16-04-13

The actual fix is replacing unicode-bidi: embed with unicode-bidi: isolate. The replaced line was introduced in #661, while fixixng other layout/rendering bugs. As far as I can tell this doesn't bring any of them back, but it's worth paying attention to that original fix.

I've added both a spec and a storybook file so screenshots of the editor are taken.


This change is Reviewable

@Nateowami Nateowami added the will require testing PR should not be merged until testers confirm testing is complete label Sep 8, 2026
@Nateowami
Nateowami deployed to screenshot_diff September 8, 2026 20:20 — with GitHub Actions Active
@codecov

codecov Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.94%. Comparing base (313ee14) to head (ce72ca7).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4089   +/-   ##
=======================================
  Coverage   80.94%   80.94%           
=======================================
  Files         664      664           
  Lines       42139    42139           
  Branches     6932     6932           
=======================================
  Hits        34111    34111           
  Misses       6865     6865           
  Partials     1163     1163           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

📸 Screenshot diff deployed! (2 changes)

View the visual diff at: https://pr-4089--sf-screenshot-diffs.netlify.app

@RaymondLuong3 RaymondLuong3 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@RaymondLuong3 reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on Nateowami).


src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/text.component.spec.ts line 285 at r1 (raw file):

    expect(verse3Marker.top).withContext('setup: verse 2 is all on one line').toEqual(verse2.top);
    expect(verse2Marker.left).toBeGreaterThanOrEqual(verse2.right);
    expect(verse3Marker.right).toBeLessThanOrEqual(verse2.left);

This is a neat way to determine that the verse markers and verse segment are in the right place. With DOM coordinates, the coordinate system starts are the top left and moves right and down. I may be worth mentioning that in a comment.

Code quote:

    expect(verse2Marker.left).toBeGreaterThanOrEqual(verse2.right);
    expect(verse3Marker.right).toBeLessThanOrEqual(verse2.left);

@RaymondLuong3 RaymondLuong3 self-assigned this Sep 8, 2026
@RaymondLuong3 RaymondLuong3 added ready to test and removed will require testing PR should not be merged until testers confirm testing is complete labels Sep 8, 2026
@Nateowami Nateowami added testing complete Testing of PR is complete and should no longer hold up merging of the PR and removed ready to test labels Sep 9, 2026
@Nateowami
Nateowami force-pushed the fix/SF-2481-rtl-bidi branch from 15d5148 to ce72ca7 Compare September 9, 2026 17:20
@Nateowami
Nateowami enabled auto-merge (squash) September 9, 2026 17:20
@Nateowami
Nateowami deployed to screenshot_diff September 9, 2026 17:29 — with GitHub Actions Active
@Nateowami
Nateowami merged commit 26d509f into master Sep 9, 2026
28 of 29 checks passed
@Nateowami
Nateowami deleted the fix/SF-2481-rtl-bidi branch September 9, 2026 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testing complete Testing of PR is complete and should no longer hold up merging of the PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants