search

nephila / djangocms-page-sitemap

django CMS page extension to handle sitemap customization
BSD 3-Clause "New" or "Revised" License
9 stars 20 forks source link

Feature/cms version 4 compatible #51

Closed vipulnarang95 closed 4 years ago

vipulnarang95 commented 4 years ago

this PR is related to Version 4 compatible changed cms_toolbars.py to allow sitemap properties menu to come under page menu changed sitemap.py to load sitemap.xml and include page properties

vipulnarang95 commented 4 years ago

this is djcms version 4 compatible

Aiky30 commented 4 years ago

@yakky We are currently working on a new Django CMS v4, this branch adds support for Django CMS 4. Are you able to create a support 4 branch for us to target? We have this in ckeditor: https://github.com/divio/djangocms-text-ckeditor/tree/support/4.0.x

Let me know if you need to discuss anything to bring you up to speed with V4.

Aiky30 commented 4 years ago

@vipulnarang95 We now have a branch to target for CMS4, can you please update this PR to point at the support 4 branch?

https://github.com/nephila/djangocms-page-sitemap/tree/support/4.0.x

vipulnarang95 commented 4 years ago

@Aiky30 updated the PR!

Aiky30 commented 4 years ago

@yakky To be able to assess if we can even achieve your requirements to make this package Django CMS v4 compatible can you comment here which changes have to be made to this PR for you to accept it?

yakky commented 4 years ago

@yakky To be able to assess if we can even achieve your requirements to make this package Django CMS v4 compatible can you comment here which changes have to be made to this PR for you to accept it?

Done! Sorry for being nitpicking, but I want ensure we have a viable and stable solution for the future

Aiky30 commented 4 years ago

@vipulnarang95 as @yakky is refusing to accept the changes I propose we close this PR and create a new app that satisfies Fidelitys requirements. We are blocked and I see no way forward here, no point wasting any more time. We can discuss with Divio if they wish to own the package or if Fidelity should own the package.

yakky commented 4 years ago

@vipulnarang95 as @yakky is refusing to accept the changes I propose we close this PR and create a new app that satisfies Fidelitys requirements. We are blocked and I see no way forward here, no point wasting any more time. We can discuss with Divio if they wish to own the package or if Fidelity should own the package.

I just asked to add fixme comments because as is, and without an overall overview of how django CMS 4 will work, it's very hard to reconcile the existing features with the proposed changes I'm ok to merge this with FIXME comments and improve the 4 support over time as it nears its release and a more open discussion about how existing features can be reimplemented in version 4 will be possible

Aiky30 commented 4 years ago

@vipulnarang95 as @yakky is refusing to accept the changes I propose we close this PR and create a new app that satisfies Fidelitys requirements. We are blocked and I see no way forward here, no point wasting any more time. We can discuss with Divio if they wish to own the package or if Fidelity should own the package.

I just asked to add fixme comments because as is, and without an overall overview of how django CMS 4 will work, it's very hard to reconcile the existing features with the proposed changes I'm ok to merge this with FIXME comments and improve the 4 support over time as it nears its release and a more open discussion about how existing features can be reimplemented in version 4 will be possible

Ok, If you are accepting the changes then we can proceed, it looked like we were stuck. We will get those implemented and run them by you ASAP. Thank you.

@vipulnarang95 Can you add the FIXME comments so that we can progress with this?

yakky commented 4 years ago

@vipulnarang95 as @yakky is refusing to accept the changes I propose we close this PR and create a new app that satisfies Fidelitys requirements. We are blocked and I see no way forward here, no point wasting any more time. We can discuss with Divio if they wish to own the package or if Fidelity should own the package.

I just asked to add fixme comments because as is, and without an overall overview of how django CMS 4 will work, it's very hard to reconcile the existing features with the proposed changes I'm ok to merge this with FIXME comments and improve the 4 support over time as it nears its release and a more open discussion about how existing features can be reimplemented in version 4 will be possible

Ok, If you are accepting the changes then we can proceed, it looked like we were stuck. We will get those implemented and run them by you ASAP. Thank you.

Sorry, I think github threading messed up the conversation abit

vipulnarang95 commented 4 years ago

@yakky @Aiky30 Added one fixme comment. Is there anything more I can help with?

yakky commented 4 years ago

@yakky @Aiky30 Added one fixme comment. Is there anything more I can help with?

Just one more fixme here https://github.com/nephila/djangocms-page-sitemap/pull/51/files#diff-1800575993541ba123fdc03a363607f4R23 and we're done for now :) Thanks

vipulnarang95 commented 4 years ago

@yakky Done

yakky commented 4 years ago

@yakky Done

:bow:

yakky commented 4 years ago

@Aiky30 can the pending conversation be resolved?

Aiky30 commented 4 years ago

@Aiky30 can the pending conversation be resolved?

@yakky I can't see any that are pending resolution, most of my comments are actually comments that appear to be outdated and I don't have the option to mark as resolved. I don't see any issues remaining. Please let me know if there is anything specifically that you can see?

yakky commented 4 years ago

I do, but I resolved myself anyway

Merging this

jonathan-s commented 4 years ago

Nice!

Sent from my mobile

On Tue, 12 May 2020, 17:58 Iacopo Spalletti, notifications@github.com wrote:

Merged #51 https://github.com/nephila/djangocms-page-sitemap/pull/51 into support/4.0.x.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nephila/djangocms-page-sitemap/pull/51#event-3328196213, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAQGYEXU6GSTZMF2A7J5VLTRRFW3ZANCNFSM4LCYVPTQ .

Aiky30 commented 4 years ago

Awesome, Thanks @yakky. Are you able to release this in the Divio Cloud on the Alpha channel? The Text CKEditor uses a dev1 version to designate that it's not classed as stable and should be treated as so: https://github.com/divio/djangocms-text-ckeditor/commit/8b684753ac261587fb2a2f8f6b0ddd35554991fc

yakky commented 4 years ago

Yes, I should be able to do that tomorrow I think

yakky commented 4 years ago

@Aiky30 released on divio cloud

Aiky30 commented 4 years ago

Awesome, thanks for your help @yakky. I'll look at what other packages you have and we can discuss a plan on how we can make the others cms v4 compatible in the near future. I'll be sure to keep you in the loop going forward. :-)