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 datetime picker and use ISO8601 format #38

Merged
merged 3 commits into from
Oct 2, 2015

Conversation

codeliner
Copy link
Member

  • user todo list is now powered by riot.js
  • added bootstrap-datetimepicker to deadline input
  • changed TodoDeadline to use ISO8601 format
  • Adjusted deadline table column to store ISO8601

- user todo list is now powered with riot.js
- added bootstrap-datetimepicker to deadline input
- changed TodoDeadline to use ISO8601 format
- Adjusted deadline table column to store ISO8601
@codeliner
Copy link
Member Author

@theDisco Date picker is added. Can you review?

I also changed the date format in the TodoDeadline and type of deadline table column as preparation for handling user TZ correctly. You now receive an ISO8601 formatted date from the client. Only missing part is, that the TodoDeadline uses the user TZ to validate the deadline.

});
</script>
</script>
<?php endif; ?>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing new line at end of file

@prolic
Copy link
Member

prolic commented Oct 1, 2015

looks good on first sight, i will check out this branch and test tomorrow.

@prolic
Copy link
Member

prolic commented Oct 1, 2015

Question: Does the bootstrap datetimer picker also work on mobile devices?

@codeliner
Copy link
Member Author

@prolic New lines are added. Sorry ;)

Regarding your question: I guess the datepicker also works on mobile devices. It is made for and works with bootstrap 3 so would be strange if it causes problems on a mobile device and it looks very mobile-ish :D. But tbh I'm not 100 % sure. It is the first time I use it. Just was looking for a datepicker with bootstrap support and this one got my attention because it includes time picking in a very smart fashion and works with moment.js which is really cool (the moment.isBefore/isAfter methods etc.)

@@ -14,7 +14,7 @@ class Version20150924204612 extends AbstractMigration
public function up(Schema $schema)
{
$todo = $schema->getTable(Table::TODO);
$todo->addColumn('deadline', 'datetime', ['default' => null, 'notnull' => false]);
$todo->addColumn('deadline', 'string', ['default' => null, 'notnull' => false, 'length' => 30]);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codeliner Why do you prefer \Doctrine\DBAL\Types::STRING over \Doctrine\DBAL\Types::DATETIMETZ? Good databases like Postgres support storing TZ in a datetime field. Storing the value as a string will prevent you from selecting TODOs which deadline is today on a database level.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question!
Basically because of this limitation

The purpose of read tables is to provide information in a query-friendly way. ISO8601 is a common standard and I would like to store datetime without the need to map it again.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was the reason why I named Postgres. My idea was to rather save the dates in UTC and handle the conversion on application side, but you are right, DateTime handles TZs really well and if we don't need the datetime type in the DB, you might as well save it as string in ISO8601 format.

@prolic
Copy link
Member

prolic commented Oct 2, 2015

Btw. Nice to have you here Wojtek.

@prolic
Copy link
Member

prolic commented Oct 2, 2015

For selecting deadlines from a specific date we could add another view (db table). We can have as many projections as we want.

@codeliner
Copy link
Member Author

@prolic 👍

@theDisco Do you agree?

If yes, I would say we keep this PR as-is and add the second projection when needed.

@prolic
Copy link
Member

prolic commented Oct 2, 2015

I can add a deadline to a todo that is already done.

@codeliner
Copy link
Member Author

@prolic We never said that this should not be possible -> #35 😄

But yeah, we should validate it. However, not in this PR but in a new one.
@theDisco Can you take over?

@theDisco
Copy link
Contributor

theDisco commented Oct 2, 2015

@prolic it is nice to be here! You are right, the same goes for notifications etc.

@codeliner sure #39

@codeliner
Copy link
Member Author

Great!

@prolic Can we merge this one?

prolic added a commit that referenced this pull request Oct 2, 2015
Add datetime picker and use ISO8601 format
@prolic prolic merged commit 2b6927e into prooph:develop Oct 2, 2015
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