Skip to content
This repository has been archived by the owner on Jun 21, 2022. It is now read-only.

Bootstrap 4 full support #122

Open
wants to merge 52 commits into
base: gh-pages
Choose a base branch
from
Open

Bootstrap 4 full support #122

wants to merge 52 commits into from

Conversation

djibe
Copy link

@djibe djibe commented May 14, 2018

No description provided.

Copy link

@elamperti elamperti left a 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 |
Copy link

@elamperti elamperti Jun 30, 2018

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?

Copy link
Author

Choose a reason for hiding this comment

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

In BS4 code yes

.clockpicker-plate {
background-color: #ededee;
border: 2px solid #dedede;
border-radius: 50%;

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 :)

Copy link
Author

Choose a reason for hiding this comment

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

I'll do it

Copy link
Author

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

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.

Copy link
Author

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

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!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants