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.

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

Formats supported languages by capitalizing them #27

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+16 −4
Open

Formats supported languages by capitalizing them #27

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2c9eb48
capitalize languages
rasheemhenry Aug 8, 2019
f6b7e8c
cleans spacing and console.logs
rasheemhenry Aug 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 3 additions & 7 deletions src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?

"português do Brasil": "pt-BR",
"български": "bg",
Expand Down Expand Up @@ -42,17 +41,14 @@ export function StripHtml(html: string) {
};

export function formatLanguages(){
let newObj = {} as any;
let nObj = {} as any;

Object.entries(SUPPORTED_LANGUAGES).forEach(
([key, value]) => {
newObj[key.charAt(0).toUpperCase() + key.slice(1)] = value;
nObj[key.charAt(0).toUpperCase() + key.slice(1)] = value;
}
);

console.log('in util');
console.log(newObj);
return newObj;
return nObj;
}

export const GITHUB_ISSUES_LINK = '//github.com/odota/underlords-web/issues';
Expand Down