Improve swap lang button for language and text swapping #482
Conversation
Pull Request Test Coverage Report for Build 7537945566
💛 - Coveralls |
|
@sushain97 , could you please review my pull request? |
|
This looks reasonable at first glance but I'll take a closer look tomorrow. I may have been wrong about the cleanest way to do this since I haven't looked at the code in a while. I appreciate the detailed PR description. |
| @@ -243,11 +243,16 @@ const WithTgtLang = ({ | |||
| const Translator = ({ mode: initialMode }: { mode?: Mode }): React.ReactElement => { | |||
| const mode: Mode = initialMode || Mode.Text; | |||
|
|
|||
| const textUrlParam = 'q'; | |||
There was a problem hiding this comment.
This shouldn't be repeated twice. It should be defined in exactly one place.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
You can define it one component (ideally the parent component) and export the constant for use in other files.
| @@ -272,6 +277,11 @@ const Translator = ({ mode: initialMode }: { mode?: Mode }): React.ReactElement | |||
|
|
|||
| const onTranslate = React.useCallback(() => window.dispatchEvent(new Event(TranslateEvent)), []); | |||
|
|
|||
| const swapLangText = () => { | |||
There was a problem hiding this comment.
Let's use React.useCallback().
There was a problem hiding this comment.
Let's also call this swapLangs. It could be used for document translation as well.
| @@ -272,6 +277,11 @@ const Translator = ({ mode: initialMode }: { mode?: Mode }): React.ReactElement | |||
|
|
|||
| const onTranslate = React.useCallback(() => window.dispatchEvent(new Event(TranslateEvent)), []); | |||
|
|
|||
| const swapLangText = () => { | |||
| setSrcText(tgtText); | |||
| setTgtText(srcText); | |||
There was a problem hiding this comment.
Let's encapsulate the language switching here as well.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks for pointing that out. Yeah, it makes no sense to make the source text the target text
|
|
||
| const swapLangText = () => { | ||
| swapLangs(); | ||
| }; |
There was a problem hiding this comment.
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?
| @@ -272,6 +277,14 @@ const Translator = ({ mode: initialMode }: { mode?: Mode }): React.ReactElement | |||
|
|
|||
| const onTranslate = React.useCallback(() => window.dispatchEvent(new Event(TranslateEvent)), []); | |||
|
|
|||
| const swapLangs = React.useCallback(() => { | |||
| setSrcText(tgtText); | |||
There was a problem hiding this comment.
Does this line ever get invoked by tests?
|
@sushain97 |
| @@ -236,8 +244,18 @@ const WithTgtLang = ({ | |||
| }, | |||
| [pair, selectedPrefs, setSelectedPrefs], | |||
| ); | |||
|
|
|||
| return children({ tgtLang, setTgtLang, recentTgtLangs, setRecentTgtLangs, pairPrefs, setPairPrefs }); | |||
| const swapLangs = React.useMemo( | |||
There was a problem hiding this comment.
This should be React.useCallback?
| @@ -84,20 +85,19 @@ it('switches between mobile and desktop', () => { | |||
|
|
|||
| describe('swapping', () => { | |||
| it('does not allow swapping when swapped pair invalid', () => { | |||
| renderLanguageSelector(); | |||
| renderLanguageSelector({ swapLangs: undefined }); | |||
There was a problem hiding this comment.
I don't think this is testing what the test description describes anymore?
| const Wrapper = () => { | ||
| const [srcText, setSrcText] = useLocalStorage('srcText', '', { | ||
| overrideValue: getUrlParam(history.location.search, textUrlParam), | ||
| }); |
There was a problem hiding this comment.
Why do we need to duplicate this code in a test...?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 [srcText, setSrcText] = useLocalStorage('srcText', '', { | ||
| overrideValue: getUrlParam(history.location.search, textUrlParam), | ||
| }); | ||
| const [tgtText, setTgtText] = React.useState(''); |
fixes #469
Description :
This pull request addresses the issue where switching languages doesn't copy already provided input and output. With the modified changes now when the languages are swapped the new input is our already provided output.
before :
https://beta.apertium.org/index.eng.html#?dir=eng-spa&q=Dogtohttps://beta.apertium.org/index.eng.html#?dir=spa-eng&q=DogAfter :
https://beta.apertium.org/index.eng.html#?dir=eng-spa&q=Dogtohttps://beta.apertium.org/index.eng.html#?dir=spa-eng&q=PerroChanges Made :
-Lifted state from the child component
TextTranslationFormto the parent componentTranslator, passing the state and its corresponding setter functions as props and wrappedsetSrcTextandsetTgtTextin the functionswapLangTextto swap the text and passed it toLanguageSelectoras a prop. Included that function inswapLangsfunction to call it whenever it is executed.-Modified
TextTranslationForm.testto accommodate the new changes made toTextTranslationForm. InLanguageSelector.test, checked whetherswapLangTextis actually getting called or not.