-
Notifications
You must be signed in to change notification settings - Fork 156
Conversation
mkoosej
commented
Aug 12, 2014
Q | A |
---|---|
Doc fix? | no |
New docs? | yes |
Applies to | dev |
Fixed tickets | symfony-cmf/menu-bundle#197 |
Do you think here is a good place to mention the extension? |
best would be the sonata admin chapter, but we seem to not have written that chapter for simple cms. yeah, lets add a |
You forgot to apply some of my comments (about line length and the headline). Did you not understand them, or did you simply forgot them? Btw, you can simply push to the |
I created a new one for the dev branch and I continue working on this PR. |
@@ -121,6 +121,108 @@ configuration in the ``sonata_admin`` section of your project configuration: | |||
|
|||
See the `Sonata Admin extension documentation`_ for more information. | |||
|
|||
MenuOptionInterface Sonata Admin Extension | |||
------------------------------------------------- |
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.
here you need to delete some -
characters so that the title line and the underline are the same length (with a fixed-width font display)
i ported the remaining comments from the old PR to here. hope its clearer now. |
@mkoosej do you add the tip for simple cms admin in this PR or a separate one? and can you please add the bit in the cmf_menu configuration reference section? i would love to merge this PR soon so that the feature is properly documented when we release cmf 1.2. |
@dbu Sorry for the hold up. I've been busy lately. I added the tip. I'm gonna add the configuration reference in a couple of hours. |
@dbu I added the configuration reference too. Please review the changes especially the php and xml configuration part. |
If ``true``, the admin extension is activated. If set to ``auto``, it will be | ||
activated only if the SonataAdminBundle is present. | ||
|
||
|
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.
extra blank line
thanks, looks good. i added a couple of minor cleanup comments. can you please adress those, and try to solve the merge conflict? and when its good, squash your commits into one? |
Kinda unrelated. But I merged the remote the upstream branch to update my forked branch and it adds some more commits. If those will not cause issue I'm gonna squash all the commits into one. |
squashed the commits and removed the merge + rebase issues in #548 thanks a lot for not only contributing but also documenting what you did, mkoosej! |
@dbu I'm glad I could help. Although I was really bad at the documentation part :) |
You're not bad. It's just that you're new to the format, our standards and documentation in general. It's kind of like writing your first PHP script. If you see it like that, you did really well! |