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

Added Localization support for About and Dashboard #95

Merged
merged 7 commits into from
Oct 25, 2023

Conversation

knightcube
Copy link
Contributor

Description

This PR Fixes Issue #90

Currently, I have added English language support for 2 pages:-

  • Dashboard
  • About

While implementing this feature I realized that support for RTL and LTR is also required as Hebrew is read from right to left but English is read from left to right.

How do I go about adding RTL support to the project using i18n as it will need changes to too many files?

Perhaps RTL support can be added later in a different PR? 🤔

Happy to know your thoughts on the changes implemented so far.

Screenshots

xDfohx16Jo

Copy link
Member

@NoamGaash NoamGaash left a comment

Choose a reason for hiding this comment

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

It looks amazing!
I'll have to approve the text with the Public Knowledge Workshop members on Monday
Thanks for your patience

src/locale/HelloLocalization.tsx Outdated Show resolved Hide resolved
@@ -2,6 +2,79 @@ import React from 'react'

const PLACEHOLDER = 'XXX'

export const TEXT_KEYS = {
Copy link
Member

Choose a reason for hiding this comment

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

is it a recommended practice?

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 didn't read about it anywhere but I just felt it would be better than typing long strings with little to no auto-complete support for keys. Putting them all in TEXT_KEYS seemed like a quicker approach for future additions too.

Copy link
Member

@NoamGaash NoamGaash Oct 6, 2023

Choose a reason for hiding this comment

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

since we use typescript, there should be autocomplete for strings when using i18n

@NoamGaash
Copy link
Member

Dear @knightcube - apologies for the delay approving the texts.
Israel is under war since Saturday, with hundreds of people killed, raped or kidnapped by the terror organization "Hamas". We are all very worried right now, and many of us volunteering for military and humanitarian causes due to the emergency.
I'll make an effort to get the texts approved once things will calm down,
thank you for your understanding
Noam

@knightcube
Copy link
Contributor Author

Hi @NoamGaash. I completely understand. Please stay safe and I really pray and hope things calm down as soon as possible.

</li>
))}
<button onClick={handleChangeLanguage}>Change Language</button>
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 hide this button for now, so we'll be able to merge it within hacktoberfest

Suggested change
<button onClick={handleChangeLanguage}>Change Language</button>
{ null && <button onClick={handleChangeLanguage}>Change Language</button>}

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @NoamGaash. Sorry for the late reply. Made the changes just now.

Copy link
Member

@NoamGaash NoamGaash left a comment

Choose a reason for hiding this comment

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

Thanks a lot! it's a great contribution

@NoamGaash NoamGaash merged commit 30c074b into hasadna:main Oct 25, 2023
6 checks passed
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