-
Notifications
You must be signed in to change notification settings - Fork 538
Bootstrap 4 full support #122
base: gh-pages
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @djibe, I'm glad to see someone worked on Bootstrap 4 support! Have you tested it in mobile? Does it work well?
I left some comments regarding your PR. My main suggestion is to ditch Bootstrap 3 support altogether to make it easier to maintain (as old versions will still be available anyway).
It appears like @weareoutman isn't around anymore, so I don't know if we will be able to merge these changes anytime soon.
Anyhow, great work here! :)
README.md
Outdated
@@ -72,7 +72,7 @@ if (something) { | |||
| default | '' | default time, 'now' or '13:14' e.g. | | |||
| placement | 'bottom' | popover placement | | |||
| align | 'left' | popover arrow align | | |||
| donetext | '完成' | done button text | | |||
| donetext | 'Done' | done button text | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has this default changed in the code as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In BS4 code yes
dist/bootstrap4-clockpicker.css
Outdated
.clockpicker-plate { | ||
background-color: #ededee; | ||
border: 2px solid #dedede; | ||
border-radius: 50%; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see a mix of tabs and spaces here, you should unify those. Perhaps check what the project has been using and set an .editorconfig
file while you are at it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
* ClockPicker v0.2.1 (http://weareoutman.github.io/clockpicker/) | ||
* Copyright 2014 Wang Shenwei. | ||
* Licensed under MIT (https://github.com/weareoutman/clockpicker/blob/gh-pages/LICENSE) | ||
* Bootstrap 4 support by djibe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should have been interesting to see the diff against the original source and I think this approach could bring several maintenance problems in the future (as bugs fixed here wouldn't be fixed in the other file, and/or features will change over time).
Judging by this, I suggest you to just override the original js with this and just leave your 0.1
version for Bootstrap 4, and whoever wants to use it with Bootstrap 3 can use version 0.0.7
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi. I'm really a noob with GitHub so I don't understand anything about your requests. Sorry.
But don't hesitate to guide me to enhance it.
True I didn't test on mobile.
So you can test it here : https://www.diabeclic.com/daemonite/bootstrap-datetimepicker-bs4daemonite.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, no problem. My suggestions were mainly two things:
- Unify spaces/tabs in the code (i.e. don't use tabs in one line, then spaces in the other; follow what the original code was using)
- Instead of naming the file
bootstrap4-clockpicker.js
, just overwrite the previous one (bootstrap-clockpicker.js
). As yours could be a new version, then users who want to use the bootstrap 3 version can use something <= 0.0.7
Don't waste much time on this, as the owner of the repo is gone and PRs probably won't be merged anyway :(
Thanks for the demo link!
Parsing input value in getTime function broke picked value when beforeHide or beforeDone callbacks were in use.
Prevented the popover position from overflowing outside the window
Removed this.parseInputValue() from getTime function
Fix on am/pm parsing on load, fix on cancel/ok button events
Fixed #8 (minify)
No description provided.