-
Notifications
You must be signed in to change notification settings - Fork 90
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
base: master
Are you sure you want to change the base?
Conversation
This seems like it would result in both |
I don't think that |
Ah, my bad. I misread the implementation. In this case, how would |
Yeah, this was something that I thought about as well. But then the main reason for using the string is to encapsulate the whole Here 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? |
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 |
There was a problem hiding this 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.
64c7848
to
4d60da0
Compare
minor changes with fixes in logic to conditionally render link
4d60da0
to
b831689
Compare
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). |
**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`.
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:-
I have added a logic to check the path of the route and conditionally render the string.
I have also tested the same.