SlideshowBlock #32

Merged
merged 38 commits into from Mar 15, 2013

Projects

None yet

3 participants

@elHornair
Member

This PR provides the possibility to create a slideshow block of child items. However the PR only cares for the structure of the slideshow, not the behaviour (otherwise we would lock the user in a certain JS library and/or jQuery plugin).
I'm not entirely sure if this should go into the BlockBundle since it's not a structural block (like ContainerBlock or ReferenceBlock) but a concrete one. Then again I wouldn't know where else to put it (reminds me on the discussion on the RssBlock).
I also wonder how we should render the images without adding a dependency on LiipImagineBundle to the BlockBundle.

What do you guys think?

Owner
dbu commented Feb 14, 2013

as said for the RssBlock, i think it is ok to provide some general-purpose blocks that do not clearly fit somewhere else in this bundle.

regarding the dependency against LiipImagineBundle, i propose to move the service into its own xml file and only conditionally load it if the imagine bundle is available. composer.json can then merely suggest LiipImagineBundle instead of require it.

@rmsint rmsint referenced this pull request in symfony-cmf/cmf-sandbox Feb 18, 2013
Closed

[WIP] sonata media example #159

@dbu dbu commented on an outdated diff Mar 5, 2013
Admin/SlideshowItemAdmin.php
@@ -0,0 +1,25 @@
+<?php
+
+namespace Symfony\Cmf\Bundle\BlockBundle\Admin;
+
+use Sonata\DoctrinePHPCRAdminBundle\Admin\Admin;
+use Sonata\AdminBundle\Form\FormMapper;
+
+class SlideshowItemAdmin extends Admin
+{
+ protected function configureFormFields(FormMapper $formMapper)
+ {
+ parent::configureFormFields($formMapper);
+
+ // image is only required when creating a new item
+ // TODO: sonata is not using one admin instance per object, so this doesnt really work - fix it
dbu
dbu Mar 5, 2013 Owner

here is still a TODO

dbu
dbu Mar 12, 2013 Owner

probably we would need to hook a form transformer. or maybe we can build this into phpcr_odm_image form type to have a sort of 'required' = 'auto' option that checks if the image is new or not. as its the most useful default behaviour, this could actually make a lot of sense.

@dbu dbu commented on an outdated diff Mar 12, 2013
Resources/config/services.xml
+ <service id="symfony_cmf.block.slideshow" class="Symfony\Cmf\Bundle\BlockBundle\Block\ContainerBlockService">
+ <tag name="sonata.block" />
+ <argument>symfony_cmf.block.slideshow</argument>
+ <argument type="service" id="templating" />
+ <argument type="service" id="sonata.block.renderer" />
+ <argument>SymfonyCmfBlockBundle:Block:block_slideshow.html.twig</argument>
+ </service>
+
+ <!-- simple block service is reused for slideshow items -->
+ <service id="symfony_cmf.block.slideshow_item" class="Symfony\Cmf\Bundle\BlockBundle\Block\SimpleBlockService">
+ <tag name="sonata.block" />
+ <argument>symfony_cmf.block.slideshow_item</argument>
+ <argument type="service" id="templating" />
+ <argument>SymfonyCmfBlockBundle:Block:block_slideshow_item.html.twig</argument>
+ </service>
+
dbu
dbu Mar 12, 2013 Owner

i wonder if we should split these out into a separate config file and have an option in the config to enable image blocks.

@dbu dbu commented on an outdated diff Mar 12, 2013
Resources/config/admin.xml
+ </service>
+
+ <service id="symfony_cmf_block.slideshow_item_admin" class="%symfony_cmf_block.slideshow_item_admin_class%">
+ <tag name="sonata.admin" manager_type="doctrine_phpcr" group="dashboard.group_content" label_catalogue="SymfonyCmfBlockBundle" label="dashboard.label_slideshow_item_block" label_translator_strategy="sonata.admin.label.strategy.underscore" />
+ <argument/>
+ <argument>%symfony_cmf_block.slideshow_item_document_class%</argument>
+ <argument>SonataAdminBundle:CRUD</argument>
+
+ <call method="setRouteBuilder">
+ <argument type="service" id="sonata.admin.route.path_info_slashes" />
+ </call>
+
+ <call method="setRoot">
+ <argument>%symfony_cmf_block.content_basepath%</argument>
+ </call>
+ </service>
dbu
dbu Mar 12, 2013 Owner

and this should be slideshow.admin.xml and could have a sub-option to activate sonata admin for the slideshows

@dbu dbu commented on an outdated diff Mar 12, 2013
Admin/MinimalSlideshowAdmin.php
+
+ // reorder children according to the form data
+ public function onPostBind(FormEvent $event)
+ {
+ /** @var $newCollection ChildrenCollection */
+ $newCollection = $event->getData()->getChildren();
+ $newCollection->clear();
+
+ foreach ($event->getForm()->get('children') as $child) {
+ $newCollection->add($child->getData());
+ }
+ }
+
+ // TODO: Item deletion doesn't work yet -> do we need to manually delete? Or does sonata call a certain method?
+ // TODO: Add doesn't work yet -> problem related to http://www.doctrine-project.org/jira/browse/PHPCR-98 ?
+ // TODO: or do we just have to add the name to the item/image?
dbu
dbu Mar 12, 2013 Owner

we should try this again once doctrine/phpcr-odm#262 is really fixed

@dbu dbu referenced this pull request in symfony-cmf/symfony-cmf-docs Mar 12, 2013
Merged

added docs on the slideshow block #104

Owner
dbu commented Mar 13, 2013

apart from the question if an image is required, this is now ready i think. any more feedback before we merge? i can live with the required not being set in some situations. it is never erronously required, only accidentally not required when you already have existing slides.

i changed the slideshow stuff to only be loaded if the configuration for symfony_cmf_block contains slideshow: true

@dbu

@elHornair this was to make the root configurable

@dbu

and this to have it in the configuration with a default

@dbu

to have the parameter available for the service definitions

@dbu

and finally this to set it on the admin class

@dbu dbu commented on an outdated diff Mar 14, 2013
Admin/MinimalSlideshowAdmin.php
+ protected function configureFormFields(FormMapper $formMapper)
+ {
+ parent::configureFormFields($formMapper);
+ $formMapper
+ ->with('form.group_general')
+ ->add('title', 'text')
+ ->with('Items')
+ ->add('children', 'sonata_type_collection',
+ array(
+ 'by_reference' => false,
+ ),
+ array(
+ 'edit' => 'inline',
+ 'inline' => 'table',
+ 'admin_code' => 'symfony_cmf_block.slideshow_item_admin',
+ 'sortable' => 'position',
dbu
dbu Mar 14, 2013 Owner

i wonder if we could move the reorder behaviour into sonata_type_collection, or maybe a phpcr_sonata_type_collection - as we found out, its quite dangerous when done naively.

@elHornair
Member

Cool, thx a lot @dbu for your notes on the configuration of the root

@dbu dbu referenced this pull request Mar 14, 2013
Closed

cleanup slideshow admin when features are implemented #38

1 of 3 tasks complete
@dbu dbu commented on an outdated diff Mar 15, 2013
Admin/Imagine/MinimalSlideshowAdmin.php
@@ -0,0 +1,101 @@
+<?php
+
+namespace Symfony\Cmf\Bundle\BlockBundle\Admin\Imagine;
+
+use Symfony\Component\Form\FormEvent;
+use Symfony\Component\Form\FormEvents;
+
+use Doctrine\Common\Util\ClassUtils;
+use Doctrine\ODM\PHPCR\ChildrenCollection;
+
+use Sonata\AdminBundle\Form\FormMapper;
+use Sonata\AdminBundle\Datagrid\ListMapper;
+use Sonata\DoctrinePHPCRAdminBundle\Admin\Admin;
+
+class MinimalSlideshowAdmin extends Admin
dbu
dbu Mar 15, 2013 Owner

sorry to be pedantic, but i think the convention would make this MinimalSlideshowBlockAdmin - other admins have the Block in their name as well.

@dbu dbu commented on an outdated diff Mar 15, 2013
Admin/Imagine/SlideshowAdmin.php
@@ -0,0 +1,34 @@
+<?php
+
+namespace Symfony\Cmf\Bundle\BlockBundle\Admin\Imagine;
+
+use Sonata\AdminBundle\Form\FormMapper;
+
+class SlideshowAdmin extends MinimalSlideshowAdmin
dbu
dbu Mar 15, 2013 Owner

same here, missing "Block"

@dbu dbu commented on the diff Mar 15, 2013
DependencyInjection/SymfonyCmfBlockExtension.php
@@ -61,10 +68,6 @@ public function load(array $configs, ContainerBuilder $container)
$container->setParameter($this->getAlias() . '.' . 'action_admin_class', $config['action_admin_class']);
}
- if (isset($config['action_admin_class'])) {
- $container->setParameter($this->getAlias() . '.' . 'action_admin_class', $config['action_admin_class']);
- }
-
dbu
dbu Mar 15, 2013 Owner

why drop this? should this not just all move to the loadSonataAdmin?

rryter
rryter Mar 15, 2013 Contributor

duplicated code

@dbu dbu commented on an outdated diff Mar 15, 2013
Document/ContainerBlock.php
{
- if ($key != null) {
- return $this->children->set($key, $child);
- }
-
return $this->children->add($child);
}
dbu
dbu Mar 15, 2013 Owner

why drop this feature of specifying the key? phpcr-odm might likely support the array key for the node name if not otherwise specified.

dbu
dbu Mar 15, 2013 Owner

i re-added this.

@dbu dbu commented on an outdated diff Mar 15, 2013
Document/ContainerBlock.php
return $this->children->add($child);
}
+
+ public function addChildren(BlockInterface $children)
+ {
+ return $this->addChild($children);
+ }
dbu
dbu Mar 15, 2013 Owner

huh? should $children not be a list of children here?

dbu
dbu Mar 15, 2013 Owner

ah no sorry, the issue is the form layer guessing and not knowing about pluralization

@dbu dbu commented on an outdated diff Mar 15, 2013
Resources/views/Block/block_slideshow.html.twig
@@ -0,0 +1,16 @@
+{% extends 'SymfonyCmfBlockBundle:Block:block_base.html.twig' %}
+
+{% block block %}
+
+ {% if childBlocks %}
+ <h3>Block title: {{ block.title }}</h3>
dbu
dbu Mar 15, 2013 Owner

why do we output "Block title: " here? i think we should check if block.title is not empty, and if it is not, output the h3 just with title

@dbu dbu merged commit 4d65f26 into master Mar 15, 2013

1 check was pending

default The Travis build is in progress
Details
@dbu dbu deleted the slideshow branch Mar 15, 2013
Owner
dbu commented Mar 15, 2013

woohoo. who would have thought that its this much effort to do this...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment