Add fragment backoffice title and improve validation #22

qsomazzi · 2016-12-01T09:44:16Z

Changelog

### Added
- Fragment "Backoffice title"

### Changed
- FragmentService `buildEditForm` now call `buildForm` to add backoffice title to each fragments
- FragmentService validate signature

Subject

The goal of this PR is to handle new backoffice title for each fragments. This improve the readability of the admin, instead of seeing only the fragment type, you will see a label related to your content. Also, the validate signature was wrong, because it's called from the article admin.

Before

After

greg0ire · 2016-12-01T10:07:33Z

Resources/public/js/fragmentList.jquery.js

@@ -134,21 +154,26 @@
            e.preventDefault();
            e.stopPropagation();

+            var sure = confirm("The fragment will be deleted after update. Are you sure?");
+
+            if (!sure) {


This variable is used only once. Why not just write

if (!confirm("The fragment will be deleted after update. Are you sure?")) { { }

greg0ire · 2016-12-01T10:07:42Z

Resources/public/js/fragmentList.jquery.js

@@ -134,21 +154,26 @@
            e.preventDefault();
            e.stopPropagation();

+            var sure = confirm("The fragment will be deleted after update. Are you sure?");


qsomazzi · 2016-12-02T08:26:21Z

Hello @sonata-project/contributors, we really need this merge by the morning, can we merge it please ?

qsomazzi · 2016-12-02T08:36:13Z

One required check can't be launched but i don't know why

greg0ire · 2016-12-02T08:57:46Z

I'll talk to @soullivaneuh about that

qsomazzi · 2016-12-02T08:58:07Z

thanks @greg0ire

greg0ire · 2016-12-02T09:00:15Z

BTW, shouldn't this be split into 2 commits? It looks like you are doing 2 separate things here.

greg0ire · 2016-12-02T09:19:50Z

@soullivaneuh unstuck StyleCI, but you have to force push to trigger it.

greg0ire · 2016-12-02T09:26:31Z

FragmentService/AbstractFragmentService.php

    {
+        if (empty($object->getBackofficeTitle())) {


Isn't there a more Symfony-ish way of doing this, by using the NotBlank constraint?

Could save you some translations…

the whole validation part is still a work in progress, for the moment we can't use symfony constraints, we are working on that on another PR. I will add the todo in doc.

qsomazzi · 2016-12-02T10:02:14Z

@greg0ire I've split it in 3 (to be more precise)

greg0ire · 2016-12-02T10:03:00Z

FragmentService/FragmentServiceInterface.php

@@ -42,8 +42,13 @@ public function buildCreateForm(FormMapper $form, FragmentInterface $fragment);
     *
     * @param FragmentInterface         $fragment
     * @param ExecutionContextInterface $context
+    /**


Uuuh… what?

greg0ire · 2016-12-02T10:04:00Z

Model/AbstractFragment.php

-     * @var int
+     * @var string
+     */
+    protected $backofficeTitle;


Should be private

can't set it private, all other properties are protected, and we used them (on fixtures for exemple)

ok leave it as is then

greg0ire · 2016-12-02T10:04:50Z

Resources/views/FragmentAdmin/form.html.twig

@@ -1,6 +1,6 @@
 {% block form %}
    {% set fragmentName = admin.fragmentServices[form.vars.data.type].name %}
-    <div data-fragment-form="{{ form.vars.id }}" data-form-tmp="true" data-formdata='{ "name": "Fragment {{ fragmentName }}", "type": "{{ fragmentName }}" }' data-errors="{{ form.vars.errors|length }}">
+    <div data-fragment-form="{{ form.vars.id }}" data-form-tmp="true" data-formdata='{ "name": "Fragment {{ fragmentName }}", "type": "{{ form.vars.data.backofficeTitle }}" }' data-errors="{{ form.vars.errors|length }}">


Linebreak this please

greg0ire · 2016-12-02T10:07:49Z

Resources/views/FragmentAdmin/form.html.twig

@@ -9,7 +9,7 @@
                    </h4>
                </div>
                <div class="box-body">
-                    {% if form.vars.errors|length > 0 %}
+                    {% if form.vars.errors is defined and form.vars.errors|length > 0 %}


form.vars.errors|default([])|lenght would be shorter, but the best would be to have this variable always defined

greg0ire · 2016-12-02T10:09:59Z

Resources/doc/reference/todo.rst

+Todo
+====
+
+This bundle is still a work in progress, we need to work more on some part.


greg0ire · 2016-12-02T10:54:10Z

Resources/views/FragmentAdmin/form.html.twig

@@ -9,7 +12,7 @@
                    </h4>
                </div>
                <div class="box-body">
-                    {% if form.vars.errors|length > 0 %}
+                    {% if form.vars.errors|default([])|lenght > 0 %}


Did you test this? Beware, this is a trick question…

honestly no, i've trust your knowledge :) will do give me 2min :p

Good answer ;) I mispellt length, and you copied it, that's why I'm asking

@greg0ire all good now :)

greg0ire · 2016-12-02T13:11:17Z

@sonata-project/contributors , please review and merge

rande · 2016-12-01T12:06:50Z

Resources/public/js/fragmentList.jquery.js

+            }
+
+            this._elementsIndex++;
+            console.log('[Sonata.Admin] [fragments|add] current index is ' + this._elementsIndex);


Admin.log and not console.log

OskarStark · 2017-03-23T09:50:48Z

I mark this PR as minor instead of major, as there is no current release

SonataCI added the review required label Dec 1, 2016

greg0ire added the major label Dec 1, 2016

greg0ire reviewed Dec 1, 2016

View reviewed changes

qsomazzi force-pushed the improve_fragments branch 3 times, most recently from 7488de3 to 740f0b6 Compare December 1, 2016 12:13

greg0ire reviewed Dec 2, 2016

View reviewed changes

qsomazzi force-pushed the improve_fragments branch 2 times, most recently from 88c53d0 to 834aeee Compare December 2, 2016 09:49

qsomazzi added 2 commits December 2, 2016 10:58

Handle fragment service validation

e88600a

Add fragment backoffice title

5dc93e3

qsomazzi force-pushed the improve_fragments branch from 834aeee to dc1f17e Compare December 2, 2016 10:01

greg0ire suggested changes Dec 2, 2016

View reviewed changes

qsomazzi force-pushed the improve_fragments branch from dc1f17e to 927b1ec Compare December 2, 2016 10:27

greg0ire reviewed Dec 2, 2016

View reviewed changes

qsomazzi force-pushed the improve_fragments branch from 927b1ec to 86e4d85 Compare December 2, 2016 13:10

greg0ire approved these changes Dec 2, 2016

View reviewed changes

Add doc and fix fragment remove issue

2f94ba6

qsomazzi force-pushed the improve_fragments branch from 86e4d85 to 2f94ba6 Compare December 2, 2016 14:46

rande approved these changes Dec 2, 2016

View reviewed changes

rande merged commit 18daa99 into sonata-project:master Dec 2, 2016

qsomazzi deleted the improve_fragments branch December 2, 2016 15:08

OskarStark added minor and removed review required major labels Mar 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fragment backoffice title and improve validation #22

Add fragment backoffice title and improve validation #22

qsomazzi commented Dec 1, 2016 •

edited

greg0ire Dec 1, 2016

greg0ire Dec 1, 2016

qsomazzi commented Dec 2, 2016

qsomazzi commented Dec 2, 2016

greg0ire commented Dec 2, 2016

qsomazzi commented Dec 2, 2016

greg0ire commented Dec 2, 2016

greg0ire commented Dec 2, 2016

greg0ire Dec 2, 2016

greg0ire Dec 2, 2016

qsomazzi Dec 2, 2016

greg0ire Dec 2, 2016

qsomazzi commented Dec 2, 2016

greg0ire Dec 2, 2016

greg0ire Dec 2, 2016

qsomazzi Dec 2, 2016

greg0ire Dec 2, 2016

greg0ire Dec 2, 2016

greg0ire Dec 2, 2016

greg0ire Dec 2, 2016

greg0ire Dec 2, 2016

qsomazzi Dec 2, 2016

greg0ire Dec 2, 2016

qsomazzi Dec 2, 2016

greg0ire commented Dec 2, 2016

rande Dec 1, 2016

OskarStark commented Mar 23, 2017 •

edited

Add fragment backoffice title and improve validation #22

Add fragment backoffice title and improve validation #22

Conversation

qsomazzi commented Dec 1, 2016 • edited

Changelog

Subject

Before

After

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qsomazzi commented Dec 2, 2016

qsomazzi commented Dec 2, 2016

greg0ire commented Dec 2, 2016

qsomazzi commented Dec 2, 2016

greg0ire commented Dec 2, 2016

greg0ire commented Dec 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qsomazzi commented Dec 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented Dec 2, 2016

Choose a reason for hiding this comment

OskarStark commented Mar 23, 2017 • edited

qsomazzi commented Dec 1, 2016 •

edited

OskarStark commented Mar 23, 2017 •

edited