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

Formats supported languages by capitalizing them #27

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rasheemhenry
Copy link

@rasheemhenry rasheemhenry commented Aug 8, 2019

fixes #19

Screen Shot 2019-08-07 at 23 15 01

{Object.entries(SUPPORTED_LANGUAGES).map(([name, lang]) => (
<a href={`/${lang}/`}>
{Object.entries(this.formatLanguages).map(([name, lang]) => (
<a key={name} href={`/${lang}/`}>
Copy link
Author

Choose a reason for hiding this comment

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

This line is being edited because React needs for each anchor to have a unique key.

@@ -11,7 +11,6 @@ export function StripHtml(html: string) {
return `${process.env.PUBLIC_URL}/images/portraits/${dotaName}_png.png`;
}

// TODO capitalize
export const SUPPORTED_LANGUAGES = {
Copy link

Choose a reason for hiding this comment

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

Is there a reason why one wouldn't just modify this object directly?

Also this object seems backwards. Short codes as the value and the human string as the key?

🤔

Copy link

Choose a reason for hiding this comment

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

This might require additional work to refactor where the language is set in redux, but it seems like a better design?

Copy link
Author

Choose a reason for hiding this comment

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

Your concerns on the object's structure, and future use are for sure valid and I invite the conversation to go over the issue outlined and scope of refactoring.
As for why not just modify the object directly, because it is currently being touched by a few other pieces in the code base (components and material-ui frame work all expecting "language": "lang-code"). I figured the swift direct approach to fix "language capitalization" (for display purpose) was to create a helper function to handle formatting for displaying purposes.

Copy link
Member

Choose a reason for hiding this comment

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

I agree it seems backwards and we could probably flip it. @albertcui do you remember if this was done for a reason? Are we indexing into this object by the human name?

@howardchung
Copy link
Member

I think we should just update the object.

OpenDota already has a copy of the correct strings, we should just paste those in:
https://github.com/odota/web/blob/master/src/lang/index.js

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.

language capitalization
3 participants