divio / djangocms-rest-api

A REST API for django CMS
MIT License
51 stars 18 forks source link

Updates #5

Closed moridyn closed 7 years ago

moridyn commented 7 years ago

Here is the updates that was made in data submission branch, but not related to it and required for api work.

czpython commented 7 years ago

@moridyn One more thing, I had a chat with @FinalAngel and we think for now we should not have a menu endpoint. The reason being that the way cms menus work is contextual, it uses the menu template tag to know which nodes to render on any given page so its not really global or predefined.

We're planning to introduce named menus at some point to the cms and then we can have an endpoint to fetch this but until then, please remove the menu related code.

moridyn commented 7 years ago

For now menu is an entry point and without it app would need to fetch all pages and build menu by it self? or how? I think we need menu anyway and we can just update the code once you have a new implementation for menu system. For now we can add more params to menu or add some info to page serializer so it would be easier to build it in app.

czpython commented 7 years ago

Hello @moridyn, I don't agree to keep the menu endpoint. It will only confuse users as its nothing like the cms menu because as mentioned the cms menu is contextual based on the navigation parameters. I suggest to remove this (confirmed with @FinalAngel) and we can later assess what would be the best approach to add support for this. We should try to keep the codebase as minimal as possible, is much harder to later adapt/extend a legacy codebase.

moridyn commented 7 years ago

Ok, I will remove and update test react app

czpython commented 7 years ago

Thanks for the modifications @moridyn. I've left one minor comment, you can go-ahead and merge this once this is addressed. Thanks again :)

narfman0 commented 7 years ago

This might should be closed :)

For posterity, I agree with @moridyn on keeping menu. It can be contextual, but tons of cases exist where people could want it the same. Disabling by default via a setting would be way better, e.g., probably a single line change. I settled on every one of my client devices parsing it. At least it is one git-revert away :)

czpython commented 7 years ago

Hello @narfman0, Thanks for your feedback.

I agree having a menu endpoint is useful and definitely something we're planning to do, our decision to leave it out for now was mainly to reduce the scope of this initial release and then continue to iterate and add more features as this application matures. In regards to the menu I feel there's many use-cases to consider before shipping such endpoint.