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

Allow passing scope variable directly to directive #141

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gabegorelick
Copy link
Collaborator

E.g. <div translate="message"></div>

Work in progress.

E.g. `<div translate="message"></div>`
@rubenv
Copy link
Owner

rubenv commented Dec 6, 2014

I'm not sure this is a good idea: users of Jade will be bitten by it.

The following piece of Jade:

span(translate) Test

Translates to:

<span translate="translate">Test</span>

There's little reason to have this syntax as an alternative to the normal version (unless I'm missing something). In those cases I prefer to have 1 good way of doing things rather than two equivalent options. More options are just plain confusing and add little value.

@gabegorelick
Copy link
Collaborator Author

Well it solves some of the issues brought up in #72. And it also might be useful to pass an options object to the directive, e.g.

$scope.translateOptions = {message: 'foo', plural: '', comment: '', context: ''};
<div translate="translateOptions"><div>

Although this isn't implemented anywhere yet, since that could make extraction difficult to say the least.

A lot of directives use this convention, so there must be a way to make it work with things like Jade.

@rubenv
Copy link
Owner

rubenv commented Dec 6, 2014

Well it solves some of the issues brought up in #72.

As a general rule I don't believe theories about performance unless it's backed up by numbers. Unless we can show that it's truly problematic (and I expect it to be negligible), it's probably best to focus on stuff that actually matters.

#72 should probably just be closed as invalid.

@gabegorelick
Copy link
Collaborator Author

Not the performance issues, which are un-tested, just some of the convenience and flickering issues. You can make a case for fixing those without the need for hard numbers.

@rubenv
Copy link
Owner

rubenv commented Dec 6, 2014

Is it our task to fix that? I mean, ng-cloak exists for a reason.

You could even solve this right now, without any changes to angular-gettext:

  1. Add a piece of code that adds a class to body when your Angular.JS app starts (probably in a .run()) somewhere
  2. Add CSS to listen to this class:
translate, [translate] {
    visibility: hidden;
}

body.loaded translate, body.loaded [translate] {
    visibility: inherit;
}

This hides all strings until the app is initialized, but keeps their sizing intact. This is actually much more desirable than display: none or starting with empty divs: much left shifting around of content.

@gabegorelick
Copy link
Collaborator Author

Is it our task to fix that?

I don't have a strong opinion one way or the other. I'm honestly more interested in whether passing a config object directly on the translate attribute would be worthwhile. I just threw this together to get started and see if it solves some of #72.

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