Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

Translatable html attributes #115

Merged
merged 4 commits into from
Feb 23, 2017
Merged

Translatable html attributes #115

merged 4 commits into from
Feb 23, 2017

Conversation

MatthiasKuehneEllerhold
Copy link
Contributor

@MatthiasKuehneEllerhold MatthiasKuehneEllerhold commented Sep 12, 2016

Some HTML attributes are marked as "translatable" (e. g. title and placeholder) and when rendering the view helper will translate the attributes value correctly.

For error messages printed via JS (Parsley to be exact) we're setting the error messages via data- attributes. In order for them to properly translate I've added the ability to add a prefix to the translatable html attributes.

This way we can register "data-translatable-" as prefix for translatable html attributes.

Impact for anybody not using this should be minimal.

I didnt write any tests or documentation for it, but I'm willing to add them if necessary.

@MatthiasKuehneEllerhold MatthiasKuehneEllerhold changed the base branch from master to develop September 12, 2016 08:16
@weierophinney
Copy link
Member

This definitely looks useful! Please write tests and provide documentation, and ping myself or @ zendframework/community-review-team to review when complete!

@MatthiasKuehneEllerhold
Copy link
Contributor Author

@weierophinney Ive added an unit test (the coverage is still going down? huh?) and some documentation.

*
* @param string $attribute
*
* @return AbstractHelper
Copy link
Member

Choose a reason for hiding this comment

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

Why this was changed in last commit from$this to AbstractHelper? I think $this is more accurate here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because @return AbstractHelper is used in the rest of the class. I've got no aversion against it, but wanted to make things homogeneous within the class.

Statistically speaking: @return $this is only used once in the whole zend-form repository: AnnotationBuilder#366

Copy link
Member

Choose a reason for hiding this comment

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

Ok then, I think we should change it, maybe not in this PR but later on, because when you extend the class IDE get confused what is actually returned from these methods.

Most time it is used in the repository return self, not return $this, both are fine with me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll agree with you, buts thats not in the scope of this PR. ;)

OT:
Is there some tool that can scan your code and hint where @return $this (or @return self) can be used? I wanted to do the switch on my projects and gave up because I dont want to look through every class manually.

*
* @param string $prefix
*
* @return AbstractHelper
Copy link
Member

Choose a reason for hiding this comment

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

As above, I think $this should be here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above. :)

@vaclavvanik
Copy link
Contributor

Will this work with Zend\Form\Factory?

@MatthiasKuehneEllerhold
Copy link
Contributor Author

MatthiasKuehneEllerhold commented Sep 15, 2016

I took a brief look at the form factory. It only creates forms out of the array notation. What I've changed are the view helpers that display the form, not the form or the factory itself.

So yes this change wont negativly affect the form factory. As per updated documentation you have to mark each HTML attribute as "translatable" in the view helper(s) that you're using. Not in the form (or factory).

If you want to set the values for the HTML attributes then you can use the form factory (as before):

$form->add(
    [
        'name'    => 'submit',
        'type'    => Input::class,
        'attributes' => [
            'data-translatable-text'  => 'Welcome!',
        ],
    ]
);

@vaclavvanik
Copy link
Contributor

@MatthiasKuehneEllerhold sure, you are right. I missed that translatable attributes are part of view helper. Thx for clarification.

@MatthiasKuehneEllerhold
Copy link
Contributor Author

@weierophinney Any news on this?

@MatthiasKuehneEllerhold
Copy link
Contributor Author

MatthiasKuehneEllerhold commented Oct 17, 2016

Ive added the ability to add default translatable attributes (and prefix) to avoid having to set one attribute to every View-Helper.
Because I've got some attributes that should be translatable with every (or "a lot of") view helper and right now I have to instantiate every view helper and set it. This would reduce the overhead necessary and increase the runtime performance.
Another possibility would be an initializer that sets the appropriate HTML Attributes upon instantiation of a view helper.

I wanted to do the same with the translator and its text domain but this should be done in the AbstractTranslatorHelper of zend-i18n. (See zend-validator where you can set an abstract translator to avoid having to set a translator for each validator.)

Any thoughts on this?

P.S.: PR for zend-i18n is here: zendframework/zend-i18n#60

@MatthiasKuehneEllerhold
Copy link
Contributor Author

MatthiasKuehneEllerhold commented Nov 2, 2016

ping @weierophinney @zendframework/community-review-team

@akrabat akrabat merged commit f495a17 into zendframework:develop Feb 23, 2017
akrabat added a commit that referenced this pull request Feb 23, 2017
…l_attribute_prefixes

Translatable html attributes
akrabat added a commit that referenced this pull request Feb 23, 2017
@akrabat
Copy link
Contributor

akrabat commented Feb 23, 2017

Thanks!

@MatthiasKuehneEllerhold MatthiasKuehneEllerhold deleted the translatable_html_attribute_prefixes branch February 23, 2017 11:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants