Skip to content

Conversation

crosspolar
Copy link
Contributor

@crosspolar crosspolar commented Mar 17, 2025

I had to replace the whole SelectDialog. My initial idea was having a Select Dialog for the next 7 days as we have it right now, but with the option to pick a date also. Maybe, however, it's better to have only the DatePicker to avoid extra clicks.

TODO

  • One backwards-compatibility logic fails: If the server-version is still using day but user with newer app picks a datetime more than 7 days ahead. I kept day in the API request, but think it potentially could reset the planned-date to the corresponding date of the same weekday within the next 7 days.
  • Datetime-picker in recipe-page
  • Squash commits

@TomBursch
Copy link
Owner

You don't need to care about introducing a breaking change in the app. I'm fine with raising the minimum backend version for this. This should make deserialization way easier.

One thing I noticed: Make sure that the fromJson method can take the output of the toJsonWithId. As for offline use, I often use these together. This means for serializing the DateTime we should probably use date.toUtc().millisecondsSinceEpoch. This makes it consistent with the Expense DateTime as well. (I know this isn't as nice in the backend since marshmallow has no field for it)

@crosspolar crosspolar force-pushed the datetime_planner_app branch from 9aa9ac0 to 21e9a20 Compare March 21, 2025 16:38
@crosspolar crosspolar force-pushed the datetime_planner_app branch from 21e9a20 to 8a43939 Compare March 21, 2025 17:28
@crosspolar
Copy link
Contributor Author

Thanks for your feedback, @TomBursch . I didn't know about the interference with toJsonWithId, so I hope I did it right now.

Another word of caution: I'm not a web developer and so I'm not always familiar with the Null Constraints of dart. Hence, please have a good look at every question or exclamation mark I use for e.g. a DateTime? data type.

@crosspolar crosspolar marked this pull request as ready for review March 21, 2025 18:12
@TomBursch TomBursch added the enhancement New feature or request label Mar 30, 2025
@TomBursch
Copy link
Owner

Hey @crosspolar, thanks for the patience. Could you rebase your changes?

@crosspolar
Copy link
Contributor Author

crosspolar commented Apr 19, 2025

@TomBursch , hope that merge is okay. I've used GitHubs "Resolve conflict" button the first time...

(I need to clean it anyway first)

@crosspolar
Copy link
Contributor Author

alright, you were faster cleaning 😄

@TomBursch
Copy link
Owner

@crosspolar yeah no worries 😄 Thanks again for this, looks really good!

@TomBursch TomBursch merged commit b0810de into TomBursch:main Apr 19, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants