[WIP] removal of duplicate about attribute in collections rendering #46
Conversation
|
CC @dbu |
| @@ -291,11 +287,14 @@ public function render($tag_name = false) | |||
| * | |||
| * @api | |||
| */ | |||
| public function renderAttributes() | |||
| public function renderAttributes($attributesToRemove = array()) | |||
There was a problem hiding this comment.
can you please call this $attributesToSkip (they are not removed, just not printed) and add a doc comment about it?
There was a problem hiding this comment.
actually the doc comment should go to the NodeInterface method.
|
@dbu @flack I think this PR is ready for the review. The README is updated and the tests have been refactored to allow to test the twig extensions with and without noautotag (CreatephpExtensionTest::testBasicTemplates). A complete example for collections rendering in twig is also available (CreatephpExtensionTest::testCollectionsTemplate). |
| @@ -224,14 +224,15 @@ name is generated by appending "_rdf" to your model name. | |||
| Inside the createphp tag, you can render the entity with the default structure | |||
| (see above), or render single fields as in the example below: | |||
|
|
|||
| {% createphp mymodel %} | |||
| <div {{ createphp_attributes(mymodel_rdf) }}> | |||
| {% createphp mymodel noautotag %} | |||
There was a problem hiding this comment.
i think the first example should be using the autotag feature, and then we add a section how to have more control over the html from the template where we explain the noautotag
There was a problem hiding this comment.
Actually if you look at https://github.com/symfony-cmf/createphp/blob/d146e4d71dd0797ffc0c4ccc859c4cf9a8e2ab1a/README.md#twig-extension, the first example is using the autotag:
{% createphp mymodel %}{{ mymodel_rdf|raw }}{% endcreatephp %}
Is it ok like this?
There was a problem hiding this comment.
ah sorry, just saw that this replaced something existing and thought there is no more example without noautotag. all fine then.
|
@flack: i put some explanatory comments into the code for you. the bigger number of changes is in that adrien uses the real metadata loader in the tests now instead of mocking, which makes the tests more reliable. @adou600 maybe you could move some of the comments i made in the PR as comments into the code so that people reading the code later see the reasoning too. |
| @@ -37,12 +37,12 @@ public function prepareObject(TypeInterface $type, $parent = null) | |||
| /** @var $meta \Doctrine\ODM\PHPCR\Mapping\ClassMetadata */ | |||
| $meta = $this->om->getClassMetaData(get_class($object)); | |||
|
|
|||
| if (!property_exists($object, $meta->parentMapping['fieldName'])) { | |||
| if (!property_exists($object, $meta->parentMapping)) { | |||
There was a problem hiding this comment.
this looks like a conflict with #47
There was a problem hiding this comment.
well it just makes #47 superfluous .. so if this PR gets merged that PR can get closed. i think its ok to include it here too.
There was a problem hiding this comment.
btw just FYI for the future you could also just cherry pick my commit and then the other PR would be automatically closed and no conflict could occur.
|
|
||
| public function __construct($title = 'the model title', | ||
| $content = 'the model content', | ||
| $subject = '/the/subject/model') { |
| * @return string the rendered attributes | ||
| */ | ||
| public function renderAttributes(); | ||
| public function renderAttributes($attributesToSkip = array()); |
There was a problem hiding this comment.
How would you feel about using a typehint here? i.e.
public function renderAttributes(array $attributesToSkip = array());just to make it a bit more robust
|
i just pushed changes according to your comments above. good to merge? |
|
Well, right now the tests fail accoring to Travis: https://travis-ci.org/flack/createphp/builds/4791902 Other than that, I think we're good :) |
|
good thing we have the tests. i did not think very far in my last commit. now it should be good, at least it was locally. |
[WIP] removal of duplicate about attribute in collections rendering
This PR try to address #29 and #44.
The idea is to add a noautotag tag in twig to allow the user not to render the enclosing html element automatically. By default, this tag is not set and the enclosing div is rendered with the rdf attributes. To avoid the about attribute duplication,
Collection::renderAttributeschecks if the parent is currently rendering and if yes removes the about attribute.Do not merge this PR yet, some changes are still to come: