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

Support for time picker. See #1 #18

Closed
wants to merge 13 commits into from
Closed

Support for time picker. See #1 #18

wants to merge 13 commits into from

Conversation

stas
Copy link

@stas stas commented Dec 11, 2012

I have some time to dedicate to time picker functionality.

This pull request is a WIP mostly to get some people feedback on the look and feel.

Right now it looks like this:

Please share some of the timepicker options you consider the best in terms of UI/UX.

@stas
Copy link
Author

stas commented Dec 12, 2012

Hey guys, when you have some time, please review this.

Here's a screenshot:

Thanks.

@Aloz1
Copy link

Aloz1 commented Dec 13, 2012

I've actually been working on the same feature, but I believe my approach looks a little nicer. If you'd like to have a look, click on my fork then download what I have done (I can't figure out how to get gh-pages working, but as soon as I have, you'll be able to click on the demo button on the readmy of my fork)

@mgibbs189
Copy link

@stas I think your latest screenshot looks great. Very nice and simple. The only major change I'd add is a way to override dropdown values. E.g. allow for 15 minute increments, or hours in 12-hour format.

@stas
Copy link
Author

stas commented Dec 13, 2012

@mgibbs189 there are a couple of new options you can use with the time picker to get that:

  • minuteStep default is 1, increment it to 15 to get less options
  • defaultTime default is false, set a Date object to change default value of the new input
  • showMeridian default is true, set false if you want a 24h format

@Aloz1 thanks for sharing your code. I took a look, and I think I'm missing any of the features above which I believe are very important for a time picker. Also, I believe you could clean-up the code a bit, some basic stats show that you basically doubled the codebase size. Here's a diffstat on my branch and yours:

$ git diff c786ec5 aloz1/master --stat
 README.md       |    6 +-
 css/pikaday.css |  120 ++++++++++++++-
 index.html      |  102 ++++++++++++-
 pika-ico.png    |  Bin 0 -> 1667 bytes
 pikaday.js      |  453 ++++++++++++++++++++++++++++++++++++++++++++++++-------
 pikaday.min.js  |    2 +-
 6 files changed, 610 insertions(+), 73 deletions(-)
$ git diff c786ec5 master --stat 
 css/pikaday.css |   15 ++++-
 pikaday.js      |  177 +++++++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 181 insertions(+), 11 deletions(-)

I need a timepicker for a project I'm working on. And it has to be as light as possible and as simple in UI/UX as a user can only expect.

@Aloz1
Copy link

Aloz1 commented Dec 13, 2012

Well, I'm still a beginner at programming (but there were a lot of modifications I had to make as well). I've only just finnished year 11. So a little bloat is still to be expected.

@stas
Copy link
Author

stas commented Dec 13, 2012

You're doing well for a beginner, please take what I wrote as a friendly advice and nothing more. Also, in future, consider splitting the commits into small chunks, it makes easier to understand the decisions behind the logic.

@Aloz1
Copy link

Aloz1 commented Dec 13, 2012

Ok, thanks :)

@scottkroyer
Copy link

If I could offer a suggestion, I would recommend splitting the bulk of the timepicker code into a separate JS library, and enable it conditionally...similar to how @dbushell conditioned the features of moment upon the presence of the library.
Progressive enhancement is awesome!

@Aloz1
Copy link

Aloz1 commented Dec 13, 2012

Thanks for the idea. Btw, mine actually works a little better on IE8 than the original (looks much more like how the other browsers render it)

@stas
Copy link
Author

stas commented Dec 13, 2012

@scottkroyer that would need big re-factoring on the original code. There are a couple of calls like this one:

this._d.setHours(0,0,0,0);

which makes it clear that the library was not designed with time support from the early beginning.

Though I think we could just remove those after @dbushell will have a look, since I changed the date comparison method in abe3379

@zcaudate
Copy link

zcaudate commented Jan 3, 2013

Is there the option to support local time. I found that the selected time does not confirm to the current time zone in the browser

@stas
Copy link
Author

stas commented Jan 3, 2013

@zcaudate what do you mean by "support for local time", there's no time-zones support if its about that.

@stas stas mentioned this pull request Feb 2, 2013
@dbushell
Copy link
Member

Hi everyone, thanks for the incredible contribution here :)

I'm not keen on pulling a time picker into the original Pikaday as it's quite a big addition and only relevant to some. I wouldn't have time to keep on top of it either!

May I suggest forking this project and I'll link to @stas and @Aloz1 in the original README in big bold letters so people who require time can easily find it.

Let me know the URLs and I'll add a new section for alternate extensions.

@stas
Copy link
Author

stas commented Feb 12, 2013

Hey, @dbushell
and welcome back! 🍰

I'm all for keeping it separate, it's just I don't really have time to maintain this. Ideally, I would leave this in a branch under the current project, and let people contribute.

@rikkert
Copy link
Member

rikkert commented Mar 23, 2013

Please let me know where the final fork lives so I can add it to the readme.

@rikkert rikkert closed this Mar 23, 2013
@stas
Copy link
Author

stas commented Mar 23, 2013

Hey @rikkert care to merge my master into a branch of this repo and mention that it's an unofficial, unmaintained version with time support and if there's anyone willing to send patches, they are always welcome?!

@rikkert
Copy link
Member

rikkert commented Mar 23, 2013

Sorry, I'd rather have it in a different network.
So we don't get any time related issues or other misunderstandings.
I wouldn't mind to link to your repo. ;)

@stas
Copy link
Author

stas commented Mar 24, 2013

Hmm, ok.
Let's do it then 👍

@rikkert
Copy link
Member

rikkert commented Mar 25, 2013

Done: e5051fb
Not sure why it is not picking up the GFM.

@owenmead
Copy link
Contributor

owenmead commented May 2, 2013

@stas and @rikkert been using Pikaday and really enjoying it, but needing time input. Seeing @stas had already implemented it I used that "extension." However, there were a handful of bugs with it plus needing the newest changes from dbushell merged into it.

Started to fix the bugs and merge in master, but soon realized this would be more work then simply rebuilding the time support with ideas taken from @stas original additions. This is exactly what I did:
https://github.com/owenmead/Pikaday

@rikkert do you want a pull request, or my guess from docs and previous discussion perhaps update the link to the newly updated repo?

@rikkert
Copy link
Member

rikkert commented May 3, 2013

Great work, I will change the README accordingly.

@owenmead
Copy link
Contributor

owenmead commented May 3, 2013

Sweet thanks. I'll try my best to keep it updated with changes to Pikaday, or if people send in issues.

@owenmead
Copy link
Contributor

owenmead commented May 5, 2014

Let is slide for a bit, but just updated the timepicker fork. Will update README in both branches shorty.

@rikkert
Copy link
Member

rikkert commented May 5, 2014

Great, if you want to change the README on this end just open a new PR.

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.

None yet

8 participants