Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Patch Add an extra ListingTemplate condition #221

Conversation

guywatson
Copy link
Contributor

I have my own implementation of a ListingPage and do not use the ListingTemplate/ListingPage module.
The extra condition ensures this logic if skipped if the ListingTemplate/ListingPage module is not being used but seperate ListingPage object has been defined.

@nyeholt
Copy link
Contributor

nyeholt commented Aug 17, 2015

Technically the if conditional should only need to check for the presence of ListingTemplate as that is the object being used (regardless of the context of what module defines it)

Separately though, it really should be making use of an extension to the NotifyUsersWorkflowAction class and using updateCMSFields for binding in the selection field rather than how it currently works. Any chance you'd be able to do that instead?

@guywatson
Copy link
Contributor Author

@nyeholt Sure I could do this. Would you like the extension applied in the advancedworkflow module .yml or the extension to simply exist in the module so people can optionally enable the extension in their own .yml files.

@nyeholt
Copy link
Contributor

nyeholt commented Aug 19, 2015

Hmm... realistically it should be an optionally enabled extension, however if it's not applied by default it'd be backwards compatible breaking, so I guess it needs to be applied by default.

@robbieaverill
Copy link
Contributor

I'm going to close this since it's unfortunately been neglected. Feel free to reopen this and rebase if you'd like it to be revived. Thanks for contributing @guywatson =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants