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.

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

Add string replacements and the string in the about modal #422

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Devesh21700Kumar
Copy link

@Devesh21700Kumar Devesh21700Kumar commented Oct 2, 2021

The issue #361

1).Added the string and created a reference instance "More_Languages" in the eng.json file along with a replacement string "more_languages" to use the anchor tag.

2). Included it the tests as well

The changes represented below:-

image

I have added a logic to check the path of the route and conditionally render the string.
I have also tested the same.

@sushain97
Copy link
Member

This seems like it would result in both beta.apertium.org and apertium.org having this string. That seems like a bit of a confusing UX?

@Devesh21700Kumar
Copy link
Author

Devesh21700Kumar commented Oct 2, 2021

This seems like it would result in both beta.apertium.org and apertium.org having this string. That seems like a bit of a confusing UX?

I don't think that apertium.org would have the same string as beta.apertium.org has a separate string instance named More_Languages in the eng.json file. Also, even the text wrapped by the anchor tag is beta.apertium.org so I guess that won't be an issue at all.

@sushain97
Copy link
Member

sushain97 commented Oct 2, 2021

Ah, my bad. I misread the implementation. In this case, how would beta.apertium.org avoid having this string entirely? It uses the same source tree as apertium.org.

@Devesh21700Kumar
Copy link
Author

Ah, my bad. I misread the implementation. In this case, how would beta.apertium.org avoid having this string entirely? It uses the same source tree as apertium.org.

Yeah, this was something that I thought about as well. But then the main reason for using the string is to encapsulate the whole Looking for more languages? Try {{more_languages}} part in {{More_Languages}}.

Here
{{more_languages}} is <a dangerouslySetInnerHTML={{ __html: 'beta.apertium.org' }} href='https://beta.apertium.org' rel='noreferrer' target='_blank'/>

and since this appeared quite modular I thought of going ahead with this implementation.

@Devesh21700Kumar
Copy link
Author

Ah, my bad. I misread the implementation. In this case, how would beta.apertium.org avoid having this string entirely? It uses the same source tree as apertium.org.

Yeah, this was something that I thought about as well. But then the main reason for using the string is to encapsulate the whole Looking for more languages? Try {{more_languages}} part in {{More_Languages}}.

Here {{more_languages}} is <a dangerouslySetInnerHTML={{ __html: 'beta.apertium.org' }} href='https://beta.apertium.org' rel='noreferrer' target='_blank'/>

and since this appeared quite modular I thought of going ahead with this implementation.

@sushain97 any follow-up on this or any workaround, you would like to suggest?

@sushain97
Copy link
Member

Maybe you could add a configuration option that enabled or disabled showing this string?

@jonorthwash requested this feature so perhaps he has an opinion on whether it matters that we show this string on both apertium.org and beta.apertium.org?

Copy link
Author

@Devesh21700Kumar Devesh21700Kumar left a comment

Choose a reason for hiding this comment

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

@sushain97 sorry for the very late follow up.
To end all confusions I have added a logic to check the path of the route and conditionally render the string.

minor changes with fixes in logic to conditionally render link
@sushain97
Copy link
Member

sushain97 commented Feb 8, 2022

To end all confusions I have added a logic to check the path of the route and conditionally render the string.

Rather than casing on the location within the frontend, we should have a configuration option that conditionally renders the URL.

Also, please write a test for any new functionality that exercises both paths (renders and doesn't render).

sushain97 pushed a commit that referenced this pull request Jan 11, 2024
**Description:**
This PR addresses issue #361 and also builds upon and supersedes the
changes proposed in PR #422.

**Changes Made:**
- Modified `config.ts` by adding `showMoreLanguagesLink` and specified
its type as `boolean` in `types.ts`. Also, added `more_languages` to
`stringReplacements`.
- Modified `AboutModal.tsx` by conditionally rendering `more_languages`
based on the value of `shouldDisplayString`, either true or false.
- In `index.test.tsx`, modified a test to check if, when the About
dialog is opened and `showMoreLanguagesLink` is set to `true`, it should
render the `more_languages`. Similarly, when `showMoreLanguagesLink` is
set to `false` and the About dialog is opened, it should not render the
`more_languages`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants