Skip to content

Commit 1758b2c

Browse files
author
Greg Trihus
committed
TT-7300 Align Mark Verses with mobile design
- Improved segment matching logic in usePlayerLogic for better accuracy. - Updated WSAudioPlayer to support new markVersesTailOpenRef prop for managing verse states. - Enhanced PassageDetailMarkVerses to handle incomplete verse rows and apply region colors dynamically. - Refactored EditReferenceDropdown to manage end verse options and suffix handling more effectively. - Improved MarkVersesTableIsMobile to allow for better interaction and visual feedback based on segment states. - Added tests to ensure correct behavior of new features and UI changes. Enhance PassageDetailMarkVersesIsMobile tests - Updated existing test to verify that the reference input is not read-only when editable. - Added new test to ensure the reference input remains read-only and disabled when the row lacks timestamps. - Improved interaction feedback for better user experience in mobile view. Recomendations from UX (AI 2nd draft) - Clicking + to add segment while playing should not stop playing but it does. - Marking verses should autosave (remove Save button) - After adding segment, the position is set to the beginning of the preceeding segment instead of the new segment. I think this used to work better. Also the second segment is yellow for active even though the position is in the first segment which is shown as completed. - When you click in a segment region (except the first) you have to press Play twice to play. - When you click add a segment boundary and click play, it plays the segment before. I think this used to work better on the last commit. - When undoing a boundary deletion. the table correctly shows the segment boundaries but the boundary does not show on the wave form. - We can mark a verse to be 1:11a but then the following verse should start with 1:11b (instead of 12) - The reset button still shows after the Mark Verses wave form and table are reset and there is nothing more to reset. Recomendations from UX (AI 1st draft) - Add segment while playing should not stop playing - Marking verses should autosave (remove Save button) - After adding segment, you have to tap Play twice to resume - Related: The design shows that the succeeding segment should be selected after adding, not the preceding. This was a specific request from you, so I'm wondering if you're unaware that this is not working? - The rows should not be editable. Please also remove the textfield-style underline for the Reference cells. I do see some merit to this for the desktop, but on mobile devices, showing the keyboard every tap is very distracting. (I did not run this local branch on my mobile device.) - The rows should be in a scrollable area, not the whole page. Moving to a scrollable table will reduce the scrollable space, so you'll at that point really need to focus on minimizing the vertical space consumed by the rest of the page. For example, the Plus/Minus row consumes a lot of height. Same for the Play/Duration row. - After you do this comment, the next important thing to do is to scroll to the row when selecting a segment in the waveform. (Right now it's a little frustrating that all the later rows are highlighted offscreen.) - The Edit Reference doesn't appear to support spanning chapters. This was a request during the design phase. Maybe it's not MVP? - The Edit Reference dropdown numbers look really large. Maybe Jiho can help with general styling and spacing these new pages. - When deleting a boundary, the waveform and table highlights get out of sync, and it appears to select the segment after where the cursor is. - I like the way the Minus button works with segment selection, even though it doesn't match the design. - Undo seems broken. It's more of a question in the design whether or not we need it, but in its current state it is not very useful. - Undo seems to undo more than the previous action. - Undo seems to not appear many times, e.g. add, delete, etc. - Undo also seems to disappear after one undo operation. Is it just saving the most recent action? It's pretty difficult to test in its current state. - Save navigates away from the page? It appears to also do a Step Complete, which should not be happening either. These issues should go away if you can accomplish autosave for this step. - I'm not really sure how to mark something as just a partial verse, e.g. 1:11a. This may be a flaw in the design. Do you know how to do this in the app? (other than manual entry in the row) - Reset does not remove the table row highlight (visual quirk) - Reset is not recoverable. Either do a confirmation dialog or make it undo-able. feat: enhance verse marking functionality with letter suffix handling and autosave improvements feat: improve playback handling during region splits to prevent unwanted seeks feat: enhance verse parsing and suffix handling in mark verses functionality Fix static reported issues. Enhance refMatch functionality to support same verse letter ranges and improve test coverage for edge cases. Added tests for valid and invalid letter suffix scenarios. Enhance PassageDetailMarkVersesIsMobile test by adding isPlaying mock function to improve playback state handling during tests. Refactor PassageDetailMarkVersesIsMobile by removing unused cell change handling and clipboard parsing functions. Updated MarkVersesTableIsMobile props to streamline verse marking functionality. Update PassageDetailMarkVerses to use dynamic RGBA values for completed and current verse backgrounds, enhancing visual feedback for verse marking functionality. Refactor PassageDetailMarkVerses to simplify row highlighting logic by utilizing applyRowHighlight function, improving code readability and maintainability.
1 parent a41883a commit 1758b2c

32 files changed

Lines changed: 2625 additions & 961 deletions

.hintrc

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,12 @@
1111
],
1212
"compat-api/html": "off",
1313
"no-inline-styles": "off",
14-
"compat-api/css": "off"
14+
"compat-api/css": "off",
15+
"axe/forms": [
16+
"default",
17+
{
18+
"select-name": "off"
19+
}
20+
]
1521
}
1622
}

localization/TranscriberAdmin-en-1.2.xliff

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10633,6 +10633,13 @@
1063310633
<context context-type="sourcefile">UserTable.tsx</context>
1063410634
</context-group>
1063510635
</trans-unit>
10636+
<trans-unit id="verse.autosaveSkipped">
10637+
<source>Autosave skipped: {0} issue(s) found.</source>
10638+
<target/>
10639+
<context-group>
10640+
<context context-type="sourcefile">PassageDetailMarkVerses.tsx</context>
10641+
</context-group>
10642+
</trans-unit>
1063610643
<trans-unit id="verse.badReferences">
1063710644
<source>ERROR: Markup contains bad references</source>
1063810645
<target/>
@@ -10689,6 +10696,13 @@
1068910696
<context context-type="sourcefile">PassageDetailMarkVerse.tsx</context>
1069010697
</context-group>
1069110698
</trans-unit>
10699+
<trans-unit id="verse.markupIssuesTitle">
10700+
<source>Verse markup issues</source>
10701+
<target/>
10702+
<context-group>
10703+
<context context-type="sourcefile">PassageDetailMarkVerses.tsx</context>
10704+
</context-group>
10705+
</trans-unit>
1069210706
<trans-unit id="verse.missingReferences">
1069310707
<source>Warning: Verses in passage not included: ({0})</source>
1069410708
<target/>
@@ -10759,6 +10773,13 @@
1075910773
<context context-type="sourcefile">PassageDetailMarkVerse.tsx</context>
1076010774
</context-group>
1076110775
</trans-unit>
10776+
<trans-unit id="verse.viewIssues">
10777+
<source>View issues</source>
10778+
<target/>
10779+
<context-group>
10780+
<context context-type="sourcefile">PassageDetailMarkVerses.tsx</context>
10781+
</context-group>
10782+
</trans-unit>
1076210783
<trans-unit id="viewMode.audioProject">
1076310784
<source>Audio Project Setup</source>
1076410785
<target/>

localization/TranscriberAdmin-en.xlf

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9112,6 +9112,12 @@
91129112
<target/>
91139113
</segment>
91149114
</unit>
9115+
<unit id="verse.autosaveSkipped">
9116+
<segment>
9117+
<source>Autosave skipped: {0} issue(s) found.</source>
9118+
<target/>
9119+
</segment>
9120+
</unit>
91159121
<unit id="verse.badReferences">
91169122
<segment>
91179123
<source>ERROR: Markup contains bad references</source>
@@ -9160,6 +9166,12 @@
91609166
<target/>
91619167
</segment>
91629168
</unit>
9169+
<unit id="verse.markupIssuesTitle">
9170+
<segment>
9171+
<source>Verse markup issues</source>
9172+
<target/>
9173+
</segment>
9174+
</unit>
91639175
<unit id="verse.missingReferences">
91649176
<segment>
91659177
<source>Warning: Verses in passage not included: ({0})</source>
@@ -9220,6 +9232,12 @@
92209232
<target/>
92219233
</segment>
92229234
</unit>
9235+
<unit id="verse.viewIssues">
9236+
<segment>
9237+
<source>View issues</source>
9238+
<target/>
9239+
</segment>
9240+
</unit>
92239241
<unit id="viewMode.audioProject">
92249242
<segment>
92259243
<source>Audio Project Setup</source>

src/renderer/public/localization/strings78399311.json

Lines changed: 0 additions & 1 deletion
This file was deleted.

src/renderer/public/localization/stringsf3868377.json

Lines changed: 1 addition & 0 deletions
Large diffs are not rendered by default.

src/renderer/src/__tests__/refMatch.test.ts

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -79,8 +79,16 @@ test('shoulc fail with beg with letter < end with letter, 1:1b-1a', () => {
7979
expect(refMatch('1:1b-1a')).toBeNull();
8080
});
8181

82-
test('should fail with beg with letter < end as letter, 1:1b-c', () => {
83-
expect(refMatch('1:1b-c')).toBeNull();
82+
test('same verse letter range 1:22a-b', () => {
83+
expect(refMatch('1:22a-b')).not.toBeNull();
84+
});
85+
86+
test('same verse letter range 1:1b-c', () => {
87+
expect(refMatch('1:1b-c')).not.toBeNull();
88+
});
89+
90+
test('should fail same verse letter range when end is before start, 1:1c-b', () => {
91+
expect(refMatch('1:1c-b')).toBeNull();
8492
});
8593

8694
test('should fail with beg as letter < end with letter, 1:b-2c', () => {

src/renderer/src/business/player/usePlayerLogic.ts

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -105,13 +105,26 @@ export const usePlayerLogic = (props: PlayerLogicProps) => {
105105
const onCurrentSegment = (segment: IRegion | undefined) => {
106106
let index = 0;
107107
if (segment && segmentsRef.current) {
108-
const segs = parseRegions(segmentsRef.current);
108+
const sorted = parseRegions(segmentsRef.current).regions.sort(
109+
(a: IRegion, b: IRegion) => a.start - b.start
110+
);
111+
const matchTol = 0.6;
109112
index =
110-
segs.regions
111-
.sort((a: IRegion, b: IRegion) => a.start - b.start)
112-
.findIndex(
113-
(r: IRegion) => r.start <= segment?.start && r.end >= segment?.end
113+
sorted.findIndex(
114+
(r: IRegion) =>
115+
Math.abs(r.start - segment.start) <= matchTol &&
116+
Math.abs(r.end - segment.end) <= matchTol
117+
) + 1;
118+
if (index <= 0) {
119+
index =
120+
sorted.findIndex(
121+
(r: IRegion, i: number) =>
122+
segment.start >= r.start - 0.01 &&
123+
(i === sorted.length - 1
124+
? segment.start <= r.end + 0.01
125+
: segment.start < r.end - 0.01)
114126
) + 1;
127+
}
115128
} else {
116129
if (setSegmentToWhole()) return;
117130
}

src/renderer/src/components/PassageDetail/PassageDetailMarkVerses.test.tsx

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,8 @@ const mockCurrentStep = 'step1';
5656
const mockSetCurrentStep = jest.fn();
5757
let mockPlayerAction: ((segment: string, init: boolean) => void) | undefined;
5858
const mockRowData: IRow[] = [];
59+
const mockShowMessage = jest.fn();
60+
const mockProjectSegmentSave = jest.fn().mockResolvedValue(undefined);
5961

6062
const passageAttributes = {
6163
sequencenum: 1,
@@ -128,6 +130,29 @@ jest.mock('./PassageDetailPlayer', () => {
128130
// }),
129131
// }));
130132
jest.mock('../../utils/logErrorService', () => jest.fn());
133+
jest.mock('../../hoc/SnackBar', () => {
134+
const actual = jest.requireActual('../../hoc/SnackBar');
135+
return {
136+
...actual,
137+
useSnackBar: () => ({
138+
showMessage: mockShowMessage,
139+
showTitledMessage: jest.fn(),
140+
messageReset: jest.fn(),
141+
}),
142+
};
143+
});
144+
jest.mock('./Internalization/useProjectSegmentSave', () => ({
145+
useProjectSegmentSave: () => mockProjectSegmentSave,
146+
}));
147+
jest.mock('../AlertDialog', () => ({
148+
__esModule: true,
149+
default: ({ jsx, title }: { jsx?: React.ReactNode; title?: string }) => (
150+
<div data-testid="issues-dialog">
151+
<h2>{title}</h2>
152+
{jsx}
153+
</div>
154+
),
155+
}));
131156
jest.mock('../../context/GlobalContext', () => ({
132157
useGlobal: (arg: string) =>
133158
arg === 'memory' ? [mockMemory, jest.fn()] : [{}, jest.fn()],
@@ -147,14 +172,18 @@ jest.mock('react-redux', () => ({
147172
reference: 'Reference',
148173
saveVerseMarkup: 'Save Verse Markup',
149174
startStop: 'Start --> Stop',
175+
autosaveSkipped: 'Autosave skipped: {0} issue(s) found.',
150176
badReferences: 'ERROR: Markup contains bad references',
151177
btNotUpdated:
152178
'WARNING: Since back translation recordings already exist, back translation segments will not be updated to line up with verse changes.',
153179
issues: 'The verse markup has issues. Do you want to continue?',
180+
markupIssuesTitle: 'Verse markup issues',
154181
missingReferences: 'Warning: Verses in passage not included: ({0})',
155182
noReferences: 'Warning: Some audio segments will not be included in verses',
156183
noSegments: 'ERROR: Some verses have no segment: ({0})',
157184
outsideReferences: 'ERROR: Some verses are outside passage: ({0})',
185+
viewIssues: 'View issues',
186+
close: 'Close',
158187
}),
159188
shallowEqual: jest.fn(),
160189
}));
@@ -463,3 +492,40 @@ test('should not add rows including refs already in table', async () => {
463492
// Assert
464493
expect(tbody.children.length).toBe(4); // 3 limits (with 3 verse refs) + 1 header
465494
});
495+
496+
test('shows warning snackbar and blocks autosave when checkRefs has issues', async () => {
497+
jest.useFakeTimers();
498+
runTest({ width: 1000 });
499+
const tbody = screen.getByTestId('verse-sheet')?.firstChild?.firstChild
500+
?.firstChild as HTMLElement;
501+
await waitFor(() => expect(tbody.children.length).toBeTruthy());
502+
503+
act(() => {
504+
if (mockPlayerAction) {
505+
mockPlayerAction(
506+
'{"regions":"[{\\"start\\":0,\\"end\\":5},{\\"start\\":5,\\"end\\":9}]"}',
507+
false
508+
);
509+
}
510+
});
511+
512+
await waitFor(() => {
513+
expect(mockShowMessage).toHaveBeenCalledWith(
514+
expect.anything(),
515+
AlertSeverity.Warning
516+
);
517+
});
518+
519+
const messageArg = mockShowMessage.mock.calls[0][0] as {
520+
props: { children: unknown[] };
521+
};
522+
const messageText = JSON.stringify(messageArg);
523+
expect(messageText).toMatch(/Autosave skipped: \d+ issue\(s\) found\./);
524+
525+
act(() => {
526+
jest.advanceTimersByTime(2000);
527+
});
528+
expect(mockProjectSegmentSave).not.toHaveBeenCalled();
529+
530+
jest.useRealTimers();
531+
});

0 commit comments

Comments
 (0)