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

Feature/fix i18n token usage #1682

Merged
merged 2 commits into from
Oct 5, 2016
Merged

Conversation

brylie
Copy link
Contributor

@brylie brylie commented Oct 3, 2016

Closes #1681

Changes

  • Remove unnecessary spaces from before _ in template helpers
  • import TAPi18n

@brylie brylie added this to the Sprint 32 milestone Oct 3, 2016
@brylie
Copy link
Contributor Author

brylie commented Oct 3, 2016

@frenchbread please review.

Ping @bajiat

@frenchbread
Copy link
Contributor

frenchbread commented Oct 3, 2016

@brylie Shouldn't we move towards common approach we are taking with templates (or template variables) like:

{ > templateName }

And have the same pattern with i18n strings?

{ _ 'template_headerText' } 

@brylie
Copy link
Contributor Author

brylie commented Oct 4, 2016

A quick search of our code found the following counts:

  • 139 results for {{>
  • 22 results for {{ >

Similarly, we tend to keep the # character next to the {{ in template block helpers:

  • 107 results for {{#
  • 41 results for {{ #

I think we should prefer the first version, keeping > and # close to the {{ for these reasons:

  • both cases are more common in our code, currently
  • the > and # characters are a bit noisy, since they are not English words (i.e. they are more to help the computer than the developer who is reading the sourcecode)
  • combining the {{> and {{# into single string, without space, makes them simpler (both to read and type)

@frenchbread
Copy link
Contributor

frenchbread commented Oct 4, 2016

both cases are more common in our code, currently

Before using eslint for js we did not care about spacing in the code. After we started using it we refactored all the code to fit it, and to have spaces.

combining the {{> and {{# into single symbols becomes a semantic unit

Disagreed here, since {{ }} are used to tell parser to operate differently than to HTML. >, # and _ are kind of operators here for specific cases.

I think it's better to keep {{ > someThing }} "with spaces" approach here, since it is:

  • Consistent
  • Easy to read

In terms of rewriting all the code to the selected approach, is just a matter of opening search bar in atom with Cmd+Shift+F and replacing one peace of the code to another for the entire project.

@brylie
Copy link
Contributor Author

brylie commented Oct 4, 2016

{{ }} are used to tell parser to operate differently than to HTML. >, # and _ are kind of operators here for specific cases.

Right, both are used primarily to help the computer (parser) to do its job.

Combining them makes it easier for the human to read, and still accomplishes the same. I.e. {{_, {{> and {{# are easy for us to read and distinguish (without spaces), while the parser can still do its job just fine.

@brylie
Copy link
Contributor Author

brylie commented Oct 4, 2016

@frenchbread I realize our opinions are different, and there is no right or wrong here.

I would like to fix the other handlebars helpers, for consistency.

In a nutshell, I am proposing we remove the space infix because:

  • handlebars spaces are unnecessary
  • handlebars spaces do not significantly increase readability (although spaces increase readability elsewhere in our code)
    • i.e. template helpers without spaces between the prefix are easy to read, since the last character is easy to distinguish
  • most template helpers in our code do not separate the handlebars ({{) from the prefix (_, >, #)

@brylie
Copy link
Contributor Author

brylie commented Oct 4, 2016

If it would help, we could do a quick vote/poll regarding this issue. Otherwise, I can just do the search/replace as part of this PR.

Ping @bajiat

@frenchbread frenchbread self-assigned this Oct 5, 2016
@frenchbread frenchbread merged commit f256b3b into develop Oct 5, 2016
@frenchbread frenchbread deleted the feature/fix-i18n-token-usage branch October 5, 2016 10:01
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.

None yet

2 participants