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.

Sign up for GitHub

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

Jump to bottom

Improve swap lang button for language and text swapping #482

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
+100 −34
Draft

Improve swap lang button for language and text swapping #482

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4bd86c8
Enhance swap-langs button to swap both languages and text inside boxes
satti-hari-krishna-reddy Jan 2, 2024
86a85cd
updated Translator.tsx
satti-hari-krishna-reddy Jan 8, 2024
dd4e28b
exported TexUrlParam
satti-hari-krishna-reddy Jan 9, 2024
77e73cf
fixed lint errors
satti-hari-krishna-reddy Jan 9, 2024
9af1056
moved swapLangs from LanguageSelector to Translator
satti-hari-krishna-reddy Jan 12, 2024
6b797ce
Merge branch 'apertium:master' into issue_469
satti-hari-krishna-reddy Jan 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions src/components/translator/LanguageSelector.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ export type Props = {
tgtLang: string;
setTgtLang: (code: string) => void;
recentTgtLangs: Array<string>;
swapLangText: () => void;

detectLangEnabled: boolean;
detectedLang: string | null;
Expand Down Expand Up @@ -423,17 +424,28 @@ const DesktopLanguageSelector = ({
};

const LanguageSelector = (props: Props): React.ReactElement => {
const { pairs, srcLang, setSrcLang, recentSrcLangs, setRecentSrcLangs, tgtLang, setTgtLang, setDetectedLang } = props;
const {
pairs,
srcLang,
setSrcLang,
recentSrcLangs,
setRecentSrcLangs,
tgtLang,
setTgtLang,
setDetectedLang,
swapLangText,
} = props;

const swapLangs = React.useMemo(
() =>
isPair(pairs, tgtLang, srcLang)
? () => {
setSrcLang(tgtLang);
setTgtLang(srcLang);
swapLangText();
}
: undefined,
[pairs, setSrcLang, setTgtLang, srcLang, tgtLang],
[pairs, setSrcLang, setTgtLang, srcLang, tgtLang, swapLangText],
);

const [detectingLang, setDetectingLang] = React.useState(false);
Expand Down
18 changes: 10 additions & 8 deletions src/components/translator/TextTranslationForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,10 @@ import { useHistory } from 'react-router-dom';
import { useMatomo } from '@datapunt/matomo-tracker-react';

import { DetectCompleteEvent, DetectEvent, PairPrefValues, TranslateEvent, baseUrlParams } from '.';
import { MaxURLLength, buildNewSearch, getUrlParam } from '../../util/url';
import { MaxURLLength, buildNewSearch } from '../../util/url';
import { APyContext } from '../../context';
import { buildUrl as buildWebpageTranslationUrl } from './WebpageTranslationForm';
import { langDirection } from '../../util/languages';
import useLocalStorage from '../../util/useLocalStorage';
import { useLocalization } from '../../util/localization';

const textUrlParam = 'q';
Expand All @@ -36,6 +35,10 @@ export type Props = {
markUnknown: boolean;
pairPrefs: PairPrefValues;
setLoading: (loading: boolean) => void;
srcText: string;
tgtText: string;
setSrcText: (text: string) => void;
setTgtText: (text: string) => void;
};

const TextTranslationForm = ({
Expand All @@ -45,6 +48,10 @@ const TextTranslationForm = ({
instantTranslation,
pairPrefs,
setLoading,
srcText,
tgtText,
setSrcText,
setTgtText,
}: Props): React.ReactElement => {
const { t } = useLocalization();
const history = useHistory();
Expand All @@ -54,11 +61,6 @@ const TextTranslationForm = ({
const srcTextareaRef = React.useRef<HTMLTextAreaElement>(null);
const tgtTextareaRef = React.useRef<HTMLTextAreaElement>(null);

const [srcText, setSrcText] = useLocalStorage('srcText', '', {
overrideValue: getUrlParam(history.location.search, textUrlParam),
});
const [tgtText, setTgtText] = React.useState('');

React.useEffect(() => {
const baseParams = baseUrlParams({ srcLang, tgtLang });
let search = buildNewSearch({ ...baseParams, [textUrlParam]: srcText });
Expand Down Expand Up @@ -130,7 +132,7 @@ const TextTranslationForm = ({
}
})();
},
[apyFetch, markUnknown, prefs, setLoading, srcLang, srcText, tgtLang, trackEvent],
[apyFetch, markUnknown, prefs, setLoading, setTgtText, srcLang, srcText, tgtLang, trackEvent],
);

const translationTimer = React.useRef<number | null>(null);
Expand Down
24 changes: 23 additions & 1 deletion src/components/translator/Translator.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,11 +243,16 @@ const WithTgtLang = ({
const Translator = ({ mode: initialMode }: { mode?: Mode }): React.ReactElement => {
const mode: Mode = initialMode || Mode.Text;

const textUrlParam = 'q';
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be repeated twice. It should be defined in exactly one place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to confirm if the suggestion is to define it globally, or if there's an alternative approach you recommend for maintaining it in a single place?

Copy link
Member

Choose a reason for hiding this comment

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

You can define it one component (ideally the parent component) and export the constant for use in other files.

const { t } = useLocalization();
const history = useHistory();
const config = React.useContext(ConfigContext);

const [loading, setLoading] = React.useState(false);
const [srcText, setSrcText] = useLocalStorage('srcText', '', {
overrideValue: getUrlParam(history.location.search, textUrlParam),
});
const [tgtText, setTgtText] = React.useState('');

const [markUnknown, setMarkUnknown] = useLocalStorage('markUnknown', false);
const [instantTranslation, setInstantTranslation] = useLocalStorage('instantTranslation', true);
Expand All @@ -272,6 +277,11 @@ const Translator = ({ mode: initialMode }: { mode?: Mode }): React.ReactElement

const onTranslate = React.useCallback(() => window.dispatchEvent(new Event(TranslateEvent)), []);

const swapLangText = () => {
Copy link
Member

Choose a reason for hiding this comment

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

Let's use React.useCallback().

Copy link
Member

Choose a reason for hiding this comment

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

Let's also call this swapLangs. It could be used for document translation as well.

setSrcText(tgtText);
setTgtText(srcText);
Copy link
Member

Choose a reason for hiding this comment

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

Let's encapsulate the language switching here as well.

Copy link
Member

Choose a reason for hiding this comment

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

Moving the target text to the source makes sense. But, making the source text the target text doesn't make sense since Apertium's translators are not necessarily bijective. That is, just because "foo" in X translates into "bar" in Y doesn't mean "bar" in Y will translate into "foo" in X.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing that out. Yeah, it makes no sense to make the source text the target text

};
Copy link
Member

Choose a reason for hiding this comment

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

This function doesn't have any purpose now? I think we can just pass swapLangs around now. It's also a bit odd that swapLangs only has one thing happen inside it. Can we also move the actual language swap there instead of src/components/translator/LanguageSelector.tsx?


return (
<Form
aria-label={t('Translate')}
Expand Down Expand Up @@ -307,12 +317,24 @@ const Translator = ({ mode: initialMode }: { mode?: Mode }): React.ReactElement
tgtLang,
detectedLang,
loading,
swapLangText,
}}
/>
{(mode === Mode.Text || !mode) && (
<>
<TextTranslationForm
{...{ instantTranslation, markUnknown, setLoading, srcLang, tgtLang, pairPrefs }}
{...{
instantTranslation,
markUnknown,
setLoading,
srcLang,
tgtLang,
pairPrefs,
srcText,
tgtText,
setSrcText,
setTgtText,
}}
/>
<Row className="mt-2 mb-3">
<Col className="d-flex d-sm-block flex-wrap translation-modes" md="6" xs="12">
Expand Down
4 changes: 3 additions & 1 deletion src/components/translator/__tests__/LanguageSelector.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ const renderLanguageSelector = (props_: Partial<Props> = {}): Props => {
detectLangEnabled: true,
detectedLang: null,
setDetectedLang: jest.fn(),
swapLangText: jest.fn(),
...props_,
};

Expand Down Expand Up @@ -90,14 +91,15 @@ describe('swapping', () => {
});

it('allow swapping when swapped pair valid', () => {
const { srcLang, tgtLang, setSrcLang, setTgtLang } = renderLanguageSelector({
const { srcLang, tgtLang, setSrcLang, setTgtLang, swapLangText } = renderLanguageSelector({
tgtLang: 'spa',
});

userEvent.click(screen.getByTestId('swap-langs-button'));

expect(setSrcLang).toHaveBeenCalledWith(tgtLang);
expect(setTgtLang).toHaveBeenCalledWith(srcLang);
expect(swapLangText).toHaveBeenCalled();
});
});

Expand Down
44 changes: 42 additions & 2 deletions src/components/translator/__tests__/TextTranslationForm.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ import userEvent from '@testing-library/user-event';

import { DetectCompleteEvent, DetectEvent, TranslateEvent } from '..';
import TextTranslationForm, { Props } from '../TextTranslationForm';
import { getUrlParam } from '../../../util/url';
import useLocalStorage from '../../../util/useLocalStorage';

const textUrlParam = 'q';

const renderTextTranslationForm = (
props_: Partial<Props> = {},
Expand All @@ -20,13 +24,34 @@ const renderTextTranslationForm = (
srcLang: 'eng',
tgtLang: 'spa',
pairPrefs: {},
srcText: '',
tgtText: '',
setSrcText: jest.fn(),
setTgtText: jest.fn(),
setLoading: jest.fn(),
...props_,
};

const Wrapper = () => {
const [srcText, setSrcText] = useLocalStorage('srcText', '', {
overrideValue: getUrlParam(history.location.search, textUrlParam),
});
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to duplicate this code in a test...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, I'm currently stuck here, and I'm not entirely sure why I went with this approach. It seems to be the only way the tests are passing, and I need your help on how to improve this setup. The functionality of TextTranslationForm is closely tied to state variables passed as props from its parent.

Copy link
Member

Choose a reason for hiding this comment

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

The problem is that the tests need to be relocated now. Moving the logic that pulls srcText from local storage from TextTranslationForm to Translator means that the tests which verify that functionality need to be moved to Translator.test.tsx.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realized that my knowledge of React testing might be a bit lacking. I plan to learn jest, and then I'll resume working on the pull request at a later point in time. I'll focus on other issues for now i.e. #405

const [tgtText, setTgtText] = React.useState('');

return (
<TextTranslationForm
{...props}
setSrcText={setSrcText}
setTgtText={setTgtText}
srcText={srcText}
tgtText={tgtText}
/>
);
};

render(
<Router history={history}>
<TextTranslationForm {...props} />
<Wrapper />
</Router>,
);

Expand Down Expand Up @@ -149,16 +174,31 @@ describe('translation', () => {
srcLang: 'eng',
tgtLang: 'spa',
pairPrefs: {},
srcText: '',
tgtText: '',
setSrcText: jest.fn(),
setTgtText: jest.fn(),
setLoading: jest.fn(),
};

const Container = () => {
const [srcLang, setSrcLang] = React.useState('eng');
const [srcText, setSrcText] = useLocalStorage('srcText', '', {
overrideValue: getUrlParam(history.location.search, textUrlParam),
});
const [tgtText, setTgtText] = React.useState('');
Copy link
Member

Choose a reason for hiding this comment

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

Same question here as above.

return (
<>
<button onClick={() => setSrcLang('cat')}>ChangeSrcLang</button>
<Router history={history}>
<TextTranslationForm {...props} srcLang={srcLang} />
<TextTranslationForm
{...props}
setSrcText={setSrcText}
setTgtText={setTgtText}
srcLang={srcLang}
srcText={srcText}
tgtText={tgtText}
/>
</Router>
</>
);
Expand Down
Loading