-
Notifications
You must be signed in to change notification settings - Fork 48
Conversation
|
||
if (false === $isNew) { | ||
if (!$isNew) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes :-)
cool, makes a lot of sense! the only bit i am unsure is the commented out line for the route. are there other reasons why you have "WIP" in the description? restarted the build to see if it works now - had a composer failure before. |
I'm planning to do more UX fixes for the menu admin. However, those doesn't need to be all in the same PR (I know you like quick merging, so +1 to merge if we all agree with the changes and travis is happy too). |
Fixed the last commit. @dbu do you like this direction I took with simplifying uri/route? The build errors are because of a BC break in Sonata 2.4 (in 2.4, they typehint against |
$linkType = $form->get('linkType')->getData(); | ||
$link = $form->get('link')->getData(); | ||
|
||
dump($linkType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems leftover debug code
regarding sonata: i think we should push as much as we can to standard form types and do as little sonata as possible. |
So, we can now start implementing Sonata 2.4 in the CMF? I thought we decided not to go for 2.4 requirements a couple of weeks back. However, going with the 2.4 requirement means we can merge a lot of my PRs about Admin UX improvements. |
https://github.com/sonata-project/SonataDoctrinePhpcrAdminBundle/blob/master/composer.json#L21 so the current master of the phpcr admin is for 2.0, and will require sonata admin 2.4. i am hesitating to merge the other PRs until we can be reasonable sure that the phpcr admin 2.0 is ready in time for the release of new versions. with what i just merged, we seem one big step closer to that however. i will try to have a look at this all this week and give my opinion. |
20211e0
to
53aa284
Compare
A Menu node does not have a URL or route. It doesn't make sense to use it.
|
Should be fixed now. |
thanks! |
Menus was, and still is, one of less easy to use admin interface. This PR tries to fix it a bit.
I changed 2 main things;