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

[Attribute] Fix hardcoded strings in attribute templates #4014

Merged
merged 1 commit into from
Jan 29, 2016

Conversation

Zales0123
Copy link
Member

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets #3984
License MIT
Doc PR

@@ -8,7 +8,8 @@
<div class="modal-body">
<div class="list-group">
{% for name, attributeType in attributeTypes %}
<a href="{{ path('sylius_backend_product_attribute_create', { 'type': name }) }}" id="{{ name }}" class="list-group-item">
{% set route = 'sylius_backend_'~subject~'_create' %}
Copy link
Contributor

Choose a reason for hiding this comment

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

I supposer the subject is product, so shouldn't it be "sylius_backend_'subject'_attribute_create"?

Copy link
Member Author

Choose a reason for hiding this comment

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

In fact, subject is "product_attribute"... perhaps variable name is not the best choice 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Then see :)

@@ -14,9 +14,9 @@
<div class="form-group">
{% set id = form.vars.label|replace(' ', '_')|lower %}
<label class="col-lg-2 control-label required" for="{{ id }}">{{ form.vars.label }}</label>
<input type="hidden" name="sylius_product[attributes][{{ count }}][attribute]" id="sylius_product_attributes_{{ count }}_attribute" value="{{ attributeId }}"/>
<input type="hidden" name="sylius_{{ subject }}[attributes][{{ count }}][attribute]" id="sylius_{{ subject }}_attributes_{{ count }}_attribute" value="{{ attributeId }}"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

The [attributes] aren't actually [values]?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it was a talk about attributes collection name some time ago, but it has not been fixed yet. Sure it should be, but I think it's not a case of this PR ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok 👍

Choose a reason for hiding this comment

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

same here, need application name

@@ -8,7 +8,8 @@
<div class="modal-body">
<div class="list-group">
{% for name, attributeType in attributeTypes %}
<a href="{{ path('sylius_backend_product_attribute_create', { 'type': name }) }}" id="{{ name }}" class="list-group-item">
{% set route = 'sylius_backend_'~subject~'_attribute_create' %}

Choose a reason for hiding this comment

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

what if you're application isn't sylius? I think you need to pass application name here

Copy link
Contributor

Choose a reason for hiding this comment

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

Hah 😄 good point Peter 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 I think, I'll follow @pjedrzejewski advice and pass metadata object into these templates.

@pjedrzejewski
Copy link
Member

@Zales0123 You can pass $this->metadata and get resource name and application name from there.

@michalmarcinkowski
Copy link
Contributor

Looks good now 👍

pjedrzejewski pushed a commit that referenced this pull request Jan 29, 2016
[Attribute] Fix hardcoded strings in attribute templates
@pjedrzejewski pjedrzejewski merged commit 74acf86 into Sylius:master Jan 29, 2016
@pjedrzejewski
Copy link
Member

Thank you Mateusz! 👍

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

4 participants