Add storybook for configure sources component#3858
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3858 +/- ##
=======================================
Coverage 81.03% 81.03%
=======================================
Files 630 630
Lines 40677 40677
Branches 6577 6601 +24
=======================================
Hits 32962 32962
+ Misses 6694 6681 -13
- Partials 1021 1034 +13 ☔ View full report in Codecov by Sentry. |
|
📸 Screenshot diff deployed! (14 changes) View the visual diff at: https://pr-3858--sf-screenshot-diffs.netlify.app |
There was a problem hiding this comment.
Pull request overview
Adds a Storybook story suite for the draft-generation “Configure Sources” UI to exercise common states and key validation/save flows in isolation.
Changes:
- Introduces
configure-sources.stories.tswith mocked service providers and generated Paratext projects/resources. - Adds multiple stories (default, pre-existing settings, mobile variants) plus interaction
playtests for selection, validation dialogs, and offline behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| hasUpdate: false, | ||
| role: SFProjectRole.ParatextObserver | ||
| }); | ||
|
|
||
| when(mockedParatextService.getResources()).thenResolve(resources); | ||
| when(mockedParatextService.getProjects()).thenResolve(projects); | ||
| when(mockedUserProjectsService.projectDocs$).thenReturn(of([args.project])); | ||
| } |
| async function selectSource( | ||
| canvasElement: HTMLElement, | ||
| fieldName: 'training_a' | 'training_b' | 'drafting', | ||
| shortName: string | ||
| ): Promise<void> { | ||
| const inputIndex = { training_a: 0, training_b: 1, drafting: 3 }[fieldName]; | ||
| const canvas = within(canvasElement); | ||
| const sourceInput = (await canvas.findAllByRole('combobox'))[inputIndex]; | ||
| await userEvent.type(sourceInput, shortName); | ||
| const item = await canvas.findByRole('option', { name: new RegExp(shortName) }); | ||
| await userEvent.click(item); | ||
| } |
| mixedSource: boolean; | ||
| online: boolean; | ||
| } | ||
|
|
||
| const defaultArgs: ConfigureSourcesComponentStoryState = { | ||
| project: blankProjectDoc, | ||
| mixedSource: true, |
marksvc
left a comment
marksvc
left a comment
There was a problem hiding this comment.
@marksvc reviewed 1 file and all commit messages, and made 9 comments.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on Nateowami).
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/configure-sources/configure-sources.stories.ts line 205 at r1 (raw file):
shortName: string ): Promise<void> { const inputIndex = { training_a: 0, training_b: 1, drafting: 3 }[fieldName];
Because the additional training source needs the user to click a button to reveal it, if storybook is not told to click the additional training source button, and we call selectSource('training_b'), won't it end up actually working with the drafting source control?
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/configure-sources/configure-sources.stories.ts line 131 at r1 (raw file):
name: 'UNK', shortName: 'UNK', languageTag: '',
A non-empty string would probably be a better "unknown" language tag, since empty string might get interpreted specially, don't you think? What do you think about something here like languageTag: 'unknown-language-code'.
Okay, after getting to the test that uses this, I see that what is desired is to have a project without a language code. So I see why you wrote "unknown language code". I think we could make this more clear by changing the comment to something like this. What do you think?
- // Add a project that has an unknown language code
+ // Add a project that has no language codesrc/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/configure-sources/configure-sources.stories.ts line 271 at r1 (raw file):
]); // // Click save and ensure we are informed that we need to confirm language codes
Extra //
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/configure-sources/configure-sources.stories.ts line 304 at r1 (raw file):
}; export const CannotSaveWithoutReferenceProject = {
(Just saying.) "ReferenceProject"
I wish we wouldn't use this terminology.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/configure-sources/configure-sources.stories.ts line 309 at r1 (raw file):
const canvas = within(canvasElement); await clearSource(canvasElement); await clearSource(canvasElement, 1);
(Just saying.) These clearSource calls are a bit opaque as to what they are touching, without looking more into the setup. But I infer what you are doing from the test name.
Ah, it looks like clearSource could be changed to be like selectSource where it receives a string with a readable name about what control it is changing.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/configure-sources/configure-sources.stories.ts line 355 at r1 (raw file):
await userEvent.click(canvas.getAllByRole('combobox')[3]); // Make sure current target Russian project can't be selected expect(canvas.queryAllByRole('option', { name: /P_ES/ })).not.toBeNull();
I am surprised for this one to be queryAllByRole. That seems like it might be a mistake?
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/configure-sources/configure-sources.stories.ts line 417 at r1 (raw file):
expect(await warning(canvasElement)).toContain('Incorrect language codes will dramatically reduce draft quality.'); await userEvent.click(await canvas.findByRole('checkbox'));
It might be a mistake that this test ends with a click?
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/configure-sources/configure-sources.stories.ts line 349 at r1 (raw file):
await userEvent.click(canvas.getAllByRole('combobox')[1]); // Make sure current target Russian project can't be selected
Okay it's starting to get too opaque. So I guess P_RU is the target project. Okay, I see from PreExistingSettings that it has P_RU as the "project", which is probably the target then. What do you think about illuminating this test more? For example, something like this change would help read it:
export const CannotSelectTargetAsASource: Story = {
...PreExistingSettings,
play: async ({ canvasElement }) => {
+ // With P_RU as the target project, the user should not be able to select P_RU as
+ // a training or drafting source.
const canvas = within(canvasElement);
- await userEvent.click(canvas.getAllByRole('combobox')[1]);
- // Make sure current target Russian project can't be selected
+ const trainingAControl = canvas.getAllByRole('combobox')[1]
+ await userEvent.click(trainingAControl);
+ // P_RU should not be an option for training source.
expect(canvas.queryByRole('option', { name: /P_ES/ })).not.toBeNull();
expect(canvas.queryByRole('option', { name: /P_RU/ })).toBeNull();
- await userEvent.click(canvas.getAllByRole('combobox')[3]);
- // Make sure current target Russian project can't be selected
+ const draftingSourceControl = canvas.getAllByRole('combobox')[3];
+ await userEvent.click(draftingSourceControl);
+ // P_RU should not be an option for drafting source.
expect(canvas.queryAllByRole('option', { name: /P_ES/ })).not.toBeNull();
expect(canvas.queryByRole('option', { name: /P_RU/ })).toBeNull();
}src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/configure-sources/configure-sources.stories.ts line 339 at r1 (raw file):
await userEvent.click(canvas.getAllByRole('combobox')[1]); expect(canvas.queryByRole('option', { name: /P_EN/ })).toBeNull(); expect(canvas.queryByRole('option', { name: /R_EN/ })).not.toBeNull();
(Just saying.) Good, I like having this not.toBeNull here, proving that we can read from the combobox list and it's not empty. Otherwise the previous assert, "You don't see thus on the page" is less meaningful.
Nateowami
force-pushed
the
feature/configure-sources-story
branch
from
eeb4ed6 to
39ead78
Compare
Nateowami
left a comment
Nateowami
left a comment
There was a problem hiding this comment.
I think I've addressed all but one comment, which I'm too tired to think about at the moment (noting that so it doesn't look like I'm just ignoring it).
@Nateowami made 7 comments and resolved 2 discussions.
Reviewable status: 0 of 1 files reviewed, 10 unresolved discussions (waiting on marksvc).
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/configure-sources/configure-sources.stories.ts line 131 at r1 (raw file):
Previously, marksvc wrote…
A non-empty string would probably be a better "unknown" language tag, since empty string might get interpreted specially, don't you think? What do you think about something here like
languageTag: 'unknown-language-code'.Okay, after getting to the test that uses this, I see that what is desired is to have a project without a language code. So I see why you wrote "unknown language code". I think we could make this more clear by changing the comment to something like this. What do you think?
- // Add a project that has an unknown language code + // Add a project that has no language code
Yes, this is about a project that doesn't have a language code properly set, an issue we've actually seen. (The last two tests reference exact Jira issues. This test was copied from the old test and adapted)
I've updated it to your suggested wording
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/configure-sources/configure-sources.stories.ts line 205 at r1 (raw file):
Previously, marksvc wrote…
Because the additional training source needs the user to click a button to reveal it, if storybook is not told to click the additional training source button, and we call selectSource('training_b'), won't it end up actually working with the drafting source control?
Good catch! I've updated it in a way that I'm not thrilled with but I think fixes it.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/configure-sources/configure-sources.stories.ts line 271 at r1 (raw file):
Previously, marksvc wrote…
Extra
//
Good catch. Done.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/configure-sources/configure-sources.stories.ts line 309 at r1 (raw file):
Previously, marksvc wrote…
(Just saying.) These clearSource calls are a bit opaque as to what they are touching, without looking more into the setup. But I infer what you are doing from the test name.
Ah, it looks like clearSource could be changed to be like selectSource where it receives a string with a readable name about what control it is changing.
I've updated it to be clearer. It was less confusing in the old tests, where the test would go to a step, and then select from the inputs shown. Once the need to go to a step no longer existed, it made selecting by index way more complicated.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/configure-sources/configure-sources.stories.ts line 355 at r1 (raw file):
Previously, marksvc wrote…
I am surprised for this one to be
queryAllByRole. That seems like it might be a mistake?
Good catch. This could never fail to be true, since it would return an empty array if nothing was matched.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/configure-sources/configure-sources.stories.ts line 417 at r1 (raw file):
Previously, marksvc wrote…
It might be a mistake that this test ends with a click?
Good catch. I tracked this down to the commit that introduced it and it appears it was just a mistake.
marksvc
left a comment
marksvc
left a comment
There was a problem hiding this comment.
@marksvc reviewed 1 file and all commit messages, made 1 comment, and resolved 5 discussions.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on Nateowami).
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/configure-sources/configure-sources.stories.ts line 230 at r2 (raw file):
const secondTrainingFieldShown = (await within(canvasElement).queryByRole('button', { name: /Add another reference project/ })) == null; return { training_a: 0, training_b: 1, drafting: secondTrainingFieldShown ? 3 : 2 }[fieldName];
Is training A the first "reference" source, and training B the second "reference" source (aka "Additional training data"), and "drafting" is the "project or .. resource you want the draft to be translated from"?
Or is one of these the "Translated project" / current project ?
Because I would have thought that this method would work something like this instead:
if the user asks for training A, return 0.
if the second training field is showing on the page:
if the user asks for training B, return 1.
if the user asks for drafting, return 2.
else: // (the second training field is NOT showing on the page)
if the user asks for drafting, return 1.
if the user asks for training B, throw an exception.
Nateowami
left a comment
Nateowami
left a comment
There was a problem hiding this comment.
@Nateowami made 1 comment.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on marksvc).
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/configure-sources/configure-sources.stories.ts line 230 at r2 (raw file):
Is training A the first "reference" source, and training B the second "reference" source (aka "Additional training data"), and "drafting" is the "project or .. resource you want the draft to be translated from"?
Yes
Or is one of these the "Translated project" / current project ?
You can't select the target project; you're just on the target project. So none of these are the current project because that's not something you can select. However, it is on the page (as a disabled field), which throws off the selector numbering.
This change is