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

Previewsdk #12

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Previewsdk #12

wants to merge 8 commits into from

Conversation

marcgoosen
Copy link

Implemented Preview functionality

@incraigulous
Copy link
Owner

Hey Marcgoosen! Sorry it took so long to respond! I love how you're moving towards injecting the client into the resources. Much simpler than my setup! Have you been using this in production? Any issues you're aware of?

Did the API change at all? I think I would like to merge this in, but I want to make sure there aren't any breaking changes I should be aware of. Also, would you mind updating the readme? That will give me a better understanding of your intended implementation so I can test this out.

@incraigulous
Copy link
Owner

Hey Marcgoosen, one more question for you!

I wanted to gauge your interest helping manage this project. I love that you took the time to dry up my existing code when building this out. I haven't had the time to give the project the attention it needs, and I would love to give someone push access that is actively using Contentful and has the chops to keep up the code quality. I'm not asking for a large time commitment. There aren't that many pull requests for this project. I just hate that it took me 3 months to get to yours. You interested?

@incraigulous
Copy link
Owner

Hey Marc! Can you explain the purpose of passing the assoc value to the json result? I would love to hear your use case.

If it's needed, I think I would rather see that method chained using get()->assoc() instead of passing in another parameter. That might require a separate response class just to have a place to chain to that method, but I think it would be worth it in terms of API simplicity. I think even getAssoc() would be better if we want to avoid chaining. Thoughts?

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

Successfully merging this pull request may close these issues.

2 participants