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

satti-hari-krishna-reddy wants to merge 6 commits into apertium:master from satti-hari-krishna-reddy:issue_469

Contributor

satti-hari-krishna-reddy commented Jan 2, 2024 •

edited

Loading

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=Dog to https://beta.apertium.org/index.eng.html#?dir=spa-eng&q=Dog

After : https://beta.apertium.org/index.eng.html#?dir=eng-spa&q=Dog to https://beta.apertium.org/index.eng.html#?dir=spa-eng&q=Perro

Changes Made :
-Lifted state from the child component TextTranslationForm to the parent component Translator, passing the state and its corresponding setter functions as props and wrapped setSrcText and setTgtText in the function swapLangText to swap the text and passed it to LanguageSelector as a prop. Included that function in swapLangs function to call it whenever it is executed.

-Modified TextTranslationForm.test to accommodate the new changes made to TextTranslationForm. In LanguageSelector.test, checked whether swapLangText is actually getting called or not.


          Enhance swap-langs button to swap both languages and text inside boxes

4bd86c8

coveralls commented Jan 2, 2024 •

edited

Loading

Pull Request Test Coverage Report for Build 7537945566

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.2%) to 97.482%

Totals
Change from base Build 7484346259:	-0.2%
Covered Lines:	1380
Relevant Lines:	1398

💛 - Coveralls

Contributor Author

satti-hari-krishna-reddy commented Jan 2, 2024

@sushain97 , could you please review my pull request?

Member

sushain97 commented Jan 3, 2024

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.

sushain97 requested changes

View reviewed changes

src/components/translator/Translator.tsx Outdated

@@ @@ -243,11 +243,16 @@ const WithTgtLang = ({ @@
               const Translator = ({ mode: initialMode }: { mode?: Mode }): React.ReactElement => {
                 const mode: Mode = initialMode || Mode.Text;
+                const textUrlParam = 'q';

Member

sushain97 Jan 7, 2024

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

Contributor Author

satti-hari-krishna-reddy Jan 8, 2024

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?

Member

sushain97 Jan 9, 2024

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

src/components/translator/Translator.tsx Outdated

		@@ -272,6 +277,11 @@ const Translator = ({ mode: initialMode }: { mode?: Mode }): React.ReactElement

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

		const swapLangText = () => {

Member

sushain97 Jan 7, 2024

Let's use React.useCallback().

Member

sushain97 Jan 7, 2024

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

src/components/translator/Translator.tsx Outdated

                 const onTranslate = React.useCallback(() => window.dispatchEvent(new Event(TranslateEvent)), []);
+                const swapLangText = () => {
+                  setSrcText(tgtText);
+                  setTgtText(srcText);

Member

sushain97 Jan 7, 2024

Let's encapsulate the language switching here as well.

Member

sushain97 Jan 7, 2024

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.

Contributor Author

satti-hari-krishna-reddy Jan 8, 2024

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

satti-hari-krishna-reddy added 3 commits

January 8, 2024 06:37


          updated Translator.tsx

86a85cd


          exported TexUrlParam

dd4e28b


          fixed lint errors

77e73cf

satti-hari-krishna-reddy requested a review from sushain97

January 10, 2024 13:00

sushain97 requested changes

View reviewed changes

src/components/translator/Translator.tsx Outdated

+                const swapLangText = () => {
+                  swapLangs();
+                };

Member

sushain97 Jan 11, 2024

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?

src/components/translator/Translator.tsx Outdated

                 const onTranslate = React.useCallback(() => window.dispatchEvent(new Event(TranslateEvent)), []);
+                const swapLangs = React.useCallback(() => {
+                  setSrcText(tgtText);

Member

sushain97 Jan 11, 2024

Does this line ever get invoked by tests?


          moved swapLangs from LanguageSelector to Translator

9af1056

Contributor Author

satti-hari-krishna-reddy commented Jan 12, 2024

@sushain97
i moved swapLangs from src/components/translator/LanguageSelector.tsx to the src/components/translator/Translator.tsx and The swapping of languages and text is functioning as expected.
However, this change has caused some issues in LanguageSelector.test.tsx. I made some adjustments to the test file to make it pass, but I think it might not be the optimal solution. Could you please take a look and help me on how to update the test accordingly?

satti-hari-krishna-reddy requested a review from sushain97

January 13, 2024 12:51

sushain97 requested changes

View reviewed changes

src/components/translator/Translator.tsx

@@ @@ -236,8 +244,18 @@ const WithTgtLang = ({ @@
                   },
                   [pair, selectedPrefs, setSelectedPrefs],
                 );
-                return children({ tgtLang, setTgtLang, recentTgtLangs, setRecentTgtLangs, pairPrefs, setPairPrefs });
+                const swapLangs = React.useMemo(

Member

sushain97 Jan 13, 2024

This should be React.useCallback?

src/components/translator/__tests__/LanguageSelector.test.tsx

@@ @@ -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 });

Member

sushain97 Jan 13, 2024

I don't think this is testing what the test description describes anymore?

src/components/translator/__tests__/TextTranslationForm.test.tsx

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

Member

sushain97 Jan 13, 2024

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

Contributor Author

satti-hari-krishna-reddy Jan 14, 2024

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.

Member

sushain97 Jan 15, 2024

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.

Contributor Author

satti-hari-krishna-reddy Jan 17, 2024 •

edited

Loading

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

src/components/translator/__tests__/TextTranslationForm.test.tsx

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

Member

sushain97 Jan 13, 2024

Same question here as above.


          Merge branch 'apertium:master' into issue_469

6b797ce

satti-hari-krishna-reddy marked this pull request as draft

March 10, 2024 13:53

satti-hari-krishna-reddy mentioned this pull request

modified " help us improve" link #485

Closed

satti-hari-krishna-reddy mentioned this pull request

modified " help us improve" link #498

Open

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet