rubenv / angular-gettext-tools

Tools for extracting/compiling angular-gettext strings.
http://angular-gettext.rocketeer.be/
MIT License
39 stars 129 forks source link

Adds support for custom attributes and strips the data- prefix #83

Closed crisbeto closed 9 years ago

gabegorelick commented 9 years ago

This is a good start, but there are a few questions we need to answer about this behavior.

  1. How does this mesh with the angular-gettext runtime? I guess you'd need to alias the directive to whatever attribute you're using instead of translate. Or do you mark every string with translate and your custom attribute?
  2. What about the other extracted info, like contexts? It looks like this patch doesn't change that behavior, so e.g. translate-context is still the marker for contexts. Maybe we should use the same attribute prefix, e.g. if you use the attribute foo to mark strings for translation, then foo-context would mark your context, instead of (or maybe in addition to?) translate-context?
crisbeto commented 9 years ago
  1. IIRC it checks if its a translate attribute, afterwards it checks if the attribute contains a data binding with the translate filter, e.g. something="{{ 'Hi' | translate }}" and only if those 2 conditions don't match, it would check if the attribute is in the list of custom attributes.
  2. Yes for contexts it would still just look for translate-context, but it could be a good idea to look for something like custom-attr-context.
gabegorelick commented 9 years ago

IIRC it checks if its a translate attribute, afterwards it checks if the attribute contains a data binding with the translate filter, e.g. `something="{{ 'Hi' | translate }}" and only if those 2 conditions don't match, it would check if the attribute is in the list of custom attributes.

Yes, but what about an extracted string that doesn't use the translate directive? It's therefore in your POT, but how would it get translated at runtime?

crisbeto commented 9 years ago

My intention for this was a little simpler: I have a use-case where the attributes get translated inside the directive logic, instead of the attribute. It saves having to add, e.g. placeholder="{{ ::'Translate me' | translate }}" all the time. The above example would instead be replaced with placeholder="Translate me" and somewhere in the directive gettextCatalog.getString(attr.placeholder).

gabegorelick commented 9 years ago

OK, that usecase makes sense. I'll let @rubenv way in on the PR though.

crisbeto commented 9 years ago

It has been a while, @rubenv do you consider this to be good for merging?

rubenv commented 9 years ago

My only concern here is that there's no way for this to support plurals and/or contexts. Not sure how to fix that though.

crisbeto commented 9 years ago

For the extraction it could look for my-custom-attribute-plural and my-custom-attribute-n and for the client side it would be up to the developer to implement it via getPlural.

rubenv commented 9 years ago

Ok, that makes sense. I do recommend we add an article with an example of how to use this correctly under the Advanced section of the dev guide: https://angular-gettext.rocketeer.be/dev-guide/

crisbeto commented 9 years ago

Cool, I'll update my PR. Is the dev guide part of the repo?

rubenv commented 9 years ago

Nope, it's over here: https://github.com/rubenv/angular-gettext-website

crisbeto commented 9 years ago

Done @rubenv, the PR for the site is at https://github.com/rubenv/angular-gettext-website/pull/15.

As an aside: I was wondering whether it would make sense for it to look also for <custom-element>. It wouldn't be much trouble to add, but I left it out since the PR was about custom attributes.

crisbeto commented 9 years ago

Sorry if I'm being annoying, but its been a while. Have you had a chance to take a look @rubenv?

rubenv commented 9 years ago

@crisbeto Sorry for the super-late reply. This is a super-quality submission, merging!

rubenv commented 9 years ago

Released v2.1.3 with this version, will also release a new version of grunt-angular-gettext so you can explicitely request it.

rubenv commented 9 years ago

Also released grunt-angular-gettext 2.1.3.

Thanks a lot!

And again, sorry for the delays, better late than never!

rubenv commented 9 years ago

Also blogged about it: https://rocketeer.be/blog/2015/08/angular-gettext-attributes/