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

Add Meal Planning Calendar Events #39

Merged
merged 3 commits into from
Dec 13, 2023

Conversation

jchadwick
Copy link
Contributor

@jchadwick jchadwick commented Nov 23, 2023

Add support for Meal Planning Calendar Events.

NOTE: current draft PR only implements reading of existing calendar events; still working on creating/updating/deleting them.

First commit in the branch also fixes a few eslint issues that were keeping me from committing; happy to make another branch/PR for those if that helps.

@jchadwick
Copy link
Contributor Author

@codetheweb seems to work pretty well so far, but would love your feedback before I go too much further.

Also, is there any value in merging this PR as read-only and then implementing the write operations in another PR, or would you rather I get it all working prior to merging?

@kevdliu
Copy link
Collaborator

kevdliu commented Dec 3, 2023

Since recipes and meal planning events seem to be closely related I think it would be better to combine the getReceipes and getMealPlanningCalendarEvents functions into one. This way only one request is needed to fetch all the data for both

@kevdliu
Copy link
Collaborator

kevdliu commented Dec 3, 2023

@codetheweb seems to work pretty well so far, but would love your feedback before I go too much further.

Also, is there any value in merging this PR as read-only and then implementing the write operations in another PR, or would you rather I get it all working prior to merging?

Since this PR is working I think it would be fine to merge it. It would make reviewing easier as well

@jchadwick jchadwick marked this pull request as ready for review December 12, 2023 04:01
@jchadwick
Copy link
Contributor Author

jchadwick commented Dec 12, 2023

If you're cool with merging the read-only, so am I.

But as we look at this I have another branch trying to get the write operations working. I've never used protobuf before so I'm a little out of my element. I'm looking at your code and I can see how it works but can't quite figure out how to write myself from scratch

Don't suppose you could point me in the direction of something that could help?

@codetheweb
Copy link
Owner

sorry for the late response, I'm fine with merging read-only for now

@jchadwick
Copy link
Contributor Author

Alright, no problem - just need to resolve your change request, but I'm not sure exactly what you're looking for.

@jchadwick
Copy link
Contributor Author

Since recipes and meal planning events seem to be closely related I think it would be better to combine the getReceipes and getMealPlanningCalendarEvents functions into one. This way only one request is needed to fetch all the data for both

I actually have another idea for this. I'll submit another PR once we get through this one.

@kevdliu kevdliu merged commit b2d06b6 into codetheweb:master Dec 13, 2023
@jchadwick jchadwick deleted the meal-planning-calendar branch December 14, 2023 03:18
@jchadwick
Copy link
Contributor Author

Since recipes and meal planning events seem to be closely related I think it would be better to combine the getReceipes and getMealPlanningCalendarEvents functions into one. This way only one request is needed to fetch all the data for both

I actually have another idea for this. I'll submit another PR once we get through this one.

Here it is: #42

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.

3 participants