feat(datepicker): initial version of datepicker component #618

maxokorokov · 2016-08-22T16:14:48Z

First datepicker version (WIP)

see the design doc
play with plunker

Is inline datepicker, not in a popup and only the month view (this will easily extended later)

Current checklist:

finish tests
decide on and cleanup CSS/HTML for day and navigation templates
extract i18n from the calendar into separate class (+ maybe use native APIs?)
finish ngModel integration (now works only one way)
add documentation
at least one demo to start with
hide exposed NgbDate from the user
replace default navigation with month/year select boxes

Please leave feedback.

WIP for #20

wesleycho · 2016-08-22T17:55:38Z

src/datepicker/ngb-calendar.ts

+export interface NgbCalendar {
+  getDaysPerWeek(): number;
+  getWeeksPerMonth(): number;
+  getFirstDayOfWeek(): number;


Would it make more sense to make these three functions getters?

Hmm, I don't think you can have getters in interfaces in Typescript

But you can have fields :-)

maxokorokov · 2016-08-25T11:54:27Z

@pkozlowski-opensource thanks for the initial review. I know the HTML markup and CSS are a mess now - it migrated here from my proof-of-concept prototype. I'll leave only the bare minimum for the layout and use bootstrap styles wherever possible.

maxokorokov · 2016-08-25T16:24:27Z

rebased to have datepicker.module.ts
cleaned up CSS significantly (removed custom colors and pixel sizes)
extracted default day template into datepicker-day-view.ts and made it use bootstrap classes
cleaned up model and argument names

sendilkumarn · 2016-08-31T16:06:57Z

src/datepicker/ngb-date.ts

@@ -0,0 +1,25 @@
+export class NgbDate {
+  static from(date: {year: number, month: number, date?: number}) {


can we make month also optional ?

Sure, but NgbDate is not user-exposed, so it wasn't necessary for now. See the public API in the NgbDatepicker

This PR should stick to minimal features as possible, that would be a feature request for the future.

maxokorokov · 2016-08-31T17:22:37Z

If anybody was following current pull request / issue #20, looks like the initial version is ready. Last changes are:

split i18n and calendar
hidden internal NgbDate from the user
introduced datepicker-navigation-selection - a simpler navigation for the first version using two select boxes for month and year. 'Zoom' style navigation (as in angular-ui) could be added later on, as it's too much already.
introduced min/maxDates and navigateTo(date) in the API
updated the plunker
more tests and docs

So it looks like this for now (see demo for minimal API example):
.

Reviews are welcome. It's big, but the minimal working component is kinda big.
Ping @pkozlowski-opensource, @wesleycho, @Foxandxss and whoever is interested

pkozlowski-opensource · 2016-09-01T09:16:11Z

demo/src/app/components/datepicker/demos/basic/datepicker-basic.ts

+
+  model;
+
+  minDate = {year: 2000, month: 0, date: 7};


Shouldn't it be day instead of date? For me date indicated the whole object. Could we please rename it?

Yeah, you're right, will rename, it will be better. Got confused by the standard js Date where there is a day field but getDate() method that returns it and getDay() that returns the day of the week...

wesleycho · 2016-09-01T15:46:35Z

src/datepicker/datepicker-navigation.ts

+    }
+  `],
+  template: `
+    <tr>


This restricts this component to being inside a tbody element - is this what we want?

I know it's restrictive as of now. Not only the ngbDatepickerNavigation, but also ngbDatepickerMonthView as well. It's because the simplest layout for the calendar I've came up with for now is a table - so all column alignment would be synchronised easily between navigation and dates, weekdays, etc. I would probably migrate to a better one if I knew how to do it easily with purer CSS.

Having said that, the only thing that's exposed now to the user is the template inside of a <td> element so the internals should be replaceable in the longer run. Now each section is just a table's <tbody>

wesleycho · 2016-09-01T15:50:31Z

I have a question about NgbCalendar - do all calendar systems have concepts of months, weeks, and days?

wesleycho · 2016-09-01T15:56:53Z

demo/src/app/components/datepicker/demos/basic/datepicker-basic.ts

+
+@Component({
+  selector: 'ngbd-datepicker-basic',
+  template: require('./datepicker-basic.html')


This should just be templateUrl: './datepicker-basic.html'

Oh, it's the script that generates the demo skeleton apparently. Good catch, thanks. Will update the script in a separate PR.

icfantv · 2016-09-01T16:44:21Z

TBH, it's been 6 weeks since I've done ng2 development and am not familiar w/ the changes past RC3. All this looks fine to me, but that should be taken w/ a grain of salt.

wesleycho · 2016-09-01T17:39:29Z

After talking with @pkozlowski-opensource, this LGTM as is minus the demo template if we want to get in. My comments were more minor/affirming the decisions being made.

maxokorokov · 2016-09-01T19:34:34Z

Thanks for taking a look, @wesleycho and @icfantv.

About calendars: I'm no calendar expert here, but I think it would be quite challenging to support calendars without day/month/year notion as well in one component. Or it will just be a parallel implementation inside.

Plus current implementation will need tuning for sure (ex. getWeeksPerMonth() in the calendar should probably be getWeeksPerMonth(month, year) as some calendars have different number of days in different months, etc)

pkozlowski-opensource · 2016-09-02T10:06:19Z

src/datepicker/datepicker-day-view.spec.ts

+import {NgbDatepickerModule} from './datepicker.module';
+import {NgbDatepickerDayView} from './datepicker-day-view';
+
+function getElement(element: HTMLElement): HTMLElement {


Could you rename it to sth more descriptive? What kind of an element we are trying to get?

pkozlowski-opensource · 2016-09-02T10:09:41Z

LGTM apart from a small nit (don't really need to be fixed) and a missing browser sniffing. Great stuff @maxokorokov - datepickers are not easy but I think that we've got really nice API and architecture in here! Let's land this baby so people can play with it.

sendilkumarn · 2016-09-02T14:37:32Z

@maxokorokov great one really 👏 👏 👏 👍

jnizet · 2016-09-02T15:39:45Z

That looks great. 👍

I come a bit late to the game, but after just looking at the demo and playing a little bit with it, here are a few remarks:

some classes aren't exported, but should be, like DayTemplateContext, NgbDatepickerI18n.
NgbDatepickerI18n should be documented in the demo page
Just as for timepicker, I wonder if we shouldn't export an interface NgbDateStruct that would give a name to the year-month-day structure used as the model of the component.
If you specify a minDate but no maxDate, you get an exception. Not a big deal, but unexpected. The exception message is very clear though 👍
Saying that months are 0-based in the documentation wouldn't hurt. That's a never-ending source of bugs
It feels strange to have mandatory limits on the navigation. I understand that the use of a select box makes it mandatory, and I don't have any idea of how to change that, but still. The very first use-case that comes to mind is the selection of a birth date, and having only 10 years before now as the limit forces the user to set a min date... and a max date (see above). Not a big deal.

Again, very good job!

maxokorokov · 2016-09-02T16:26:28Z

Thanks for the review @jnizet

DayTemplateContext is an interface purely for documentation, user doesn't need it
concerning min/max dates: default values are -10 and +10 yrs just to provide some default values and yes, you have to provide some limits for select boxes...
There is a different kind of 'zoom' navigation that might be implemented as in here for instance that will make navigation infinite. Can be done later
will allow setting only one of minDate or maxDate and leaving the second one as default value, you're right about this one

icfantv · 2016-09-02T18:30:57Z

IMO, this is good. @maxokorokov have you made [do you want to make] any changes per @jnizet's recommendations, specifically, the last bullet above? If not, I'm inclined to merge this. We can tweak the padding after it lands.

maxokorokov · 2016-09-02T21:35:43Z

Amended ie9 cursor test ignore and min/maxDate are completely optional now as in the comment above

Closes ng-bootstrap#20

maxokorokov force-pushed the datepicker branch 2 times, most recently from e6d1e3e to 9c2ef50 Compare August 22, 2016 17:02

wesleycho reviewed Aug 22, 2016
View reviewed changes

maxokorokov force-pushed the datepicker branch from 9c2ef50 to 70a9eb8 Compare August 25, 2016 16:19

maxokorokov force-pushed the datepicker branch 9 times, most recently from 43eb02e to b5b1053 Compare August 31, 2016 15:46

sendilkumarn reviewed Aug 31, 2016
View reviewed changes

maxokorokov force-pushed the datepicker branch from b5b1053 to c9164dd Compare August 31, 2016 16:56

maxokorokov changed the title ~~WIP: feat(datepicker): adds datepicker component~~ feat(datepicker): initial version of datepicker component Aug 31, 2016

pkozlowski-opensource reviewed Sep 1, 2016
View reviewed changes

wesleycho reviewed Sep 1, 2016
View reviewed changes

maxokorokov force-pushed the datepicker branch 2 times, most recently from 2dd9931 to 4d2c8f8 Compare September 2, 2016 09:11

pkozlowski-opensource added this to the alpha.4 milestone Sep 2, 2016

pkozlowski-opensource reviewed Sep 2, 2016
View reviewed changes

maxokorokov mentioned this pull request Sep 2, 2016

test: ability to exclude certain tests for certain browsers #663

Closed

maxokorokov force-pushed the datepicker branch 3 times, most recently from 3997fd6 to e3e256f Compare September 2, 2016 21:30

maxokorokov added needs: merging and removed needs: merging labels Sep 2, 2016

feat(datepicker): initial version of datepicker component

c6fde0c

Closes ng-bootstrap#20

maxokorokov force-pushed the datepicker branch from e3e256f to c6fde0c Compare September 2, 2016 21:40

maxokorokov added the component: datepicker label Sep 2, 2016

maxokorokov merged commit 9e8c5c0 into ng-bootstrap:master Sep 2, 2016

maxokorokov deleted the datepicker branch March 23, 2018 23:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(datepicker): initial version of datepicker component #618

feat(datepicker): initial version of datepicker component #618

maxokorokov commented Aug 22, 2016 •

edited

wesleycho Aug 22, 2016

maxokorokov Aug 23, 2016

pkozlowski-opensource Aug 25, 2016

maxokorokov commented Aug 25, 2016

maxokorokov commented Aug 25, 2016

sendilkumarn Aug 31, 2016

maxokorokov Aug 31, 2016 •

edited

wesleycho Aug 31, 2016

maxokorokov commented Aug 31, 2016

pkozlowski-opensource Sep 1, 2016

maxokorokov Sep 1, 2016

wesleycho Sep 1, 2016

maxokorokov Sep 1, 2016

wesleycho commented Sep 1, 2016

wesleycho Sep 1, 2016

maxokorokov Sep 1, 2016

icfantv commented Sep 1, 2016

wesleycho commented Sep 1, 2016

maxokorokov commented Sep 1, 2016

pkozlowski-opensource Sep 2, 2016

pkozlowski-opensource commented Sep 2, 2016

sendilkumarn commented Sep 2, 2016

jnizet commented Sep 2, 2016

maxokorokov commented Sep 2, 2016

icfantv commented Sep 2, 2016

maxokorokov commented Sep 2, 2016

		@@ -0,0 +1,25 @@
		export class NgbDate {
		static from(date: {year: number, month: number, date?: number}) {

feat(datepicker): initial version of datepicker component #618

feat(datepicker): initial version of datepicker component #618

Conversation

maxokorokov commented Aug 22, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxokorokov commented Aug 25, 2016

maxokorokov commented Aug 25, 2016

Choose a reason for hiding this comment

maxokorokov Aug 31, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxokorokov commented Aug 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wesleycho commented Sep 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icfantv commented Sep 1, 2016

wesleycho commented Sep 1, 2016

maxokorokov commented Sep 1, 2016

Choose a reason for hiding this comment

pkozlowski-opensource commented Sep 2, 2016

sendilkumarn commented Sep 2, 2016

jnizet commented Sep 2, 2016

maxokorokov commented Sep 2, 2016

icfantv commented Sep 2, 2016

maxokorokov commented Sep 2, 2016

maxokorokov commented Aug 22, 2016 •

edited

maxokorokov Aug 31, 2016 •

edited