Adding new 'cmf_media_file' form type #137
Conversation
|
One build is failing due to a Twig compiler problem (Twig v1.12.0-RC1). It generates a compiled template with a syntax error. |
| {% block cmf_media_image_widget_preview %} | ||
| {% if form.vars.data and form.vars.data.id is defined and form.vars.data.id %} | ||
| <img src="{{ cmf_media_display_url(form.vars.data, { imagine_filter : imagine_filter }) }}" alt="" {% if not imagine_filter %}width="100"{% endif %} /> | ||
| {% endif %} | ||
| {% endblock %} | ||
| {{ form_widget(form) }} |
There was a problem hiding this comment.
i fear changing the order here is a BC break for users of the bundle.
There was a problem hiding this comment.
OK I will revert it to the original position to avoid breaking compatibility.
Anyway, to me it makes more sense having the preview of the uploaded image before the upload button (for replacing it). I could possibly propose this in a PR for a future version of the bundle.
|
hey, i like this one very much! the refactoring is great, and you even added tests for this! the test failure is most likely unrelated. we do one build with --prefer-lowest exactly for the reason to see if things work or if we need to fix the minimum requirements in composer.json. i created #138 about that - we won't block this pull request because of it. |
|
@dbu thank you for your review. I've addressed your points, and hope now the code is better documented. |
|
great job, thanks a lot! i created the doc issue, please do a pull request for the documentation. it can be short, but at least the user should find a hint that this exists. |
|
please rebase on master to get the tests fixed. |
|
Rebased (and squashed). All test are ok now. |
Adding new 'cmf_media_file' form type
|
good job, thanks! |
To do:
As discussed in #99, the MediaBundle seems to miss a 'cmf_media_file' form type, like the already defined 'cmf_media_image'.
This form type would be useful when a file must be embedded/attached to a container document, and maybe useful also for sonata admin.