DatePicker: add attribute defaultTime #9094

nighca · 2018-01-02T03:28:55Z

Please make sure these boxes are checked before submitting your PR, thank you!

Make sure you follow Element's contributing guide (中文 | English | Español).
Make sure you are merging your commits to dev branch.
Add some descriptions and refer relative issues for you PR.

Details: #7646

nighca · 2018-01-02T03:31:13Z

@wacky6 @Leopoldthecoder Plz review. And, should I update document files in this PR?

wacky6 · 2018-01-02T11:49:48Z

CI failed for some reasons. Can you check it?

wacky6 · 2018-01-02T11:52:51Z

Yes, you can update the document. Just write English or Chinese (or both), the Spanish one will be translated afterwards.

I prefer a separate section explaining the two new behaviors (do not reset & reset to defaultTime). Each should have a demo.

nighca · 2018-01-02T11:56:34Z

Ok, I will check it and add docs when free.

nighca · 2018-01-11T16:47:18Z

@wacky6 I fixed the CI. Also I added some docs and an example for the attribute defaultTime. Plz review.

I've run into a strange problem that the test case fails in Travis if I use DELAY here, while it passes on my own computer. I have to use a longer delay time(DELAY * 2) to make it work in Travis environment.

Can you plz check if it is fine to do so, or if there is any better solution.

wacky6 · 2018-01-13T13:24:05Z

The cause of DELAY * 2 is strange. createVue / createTest should not require additional time to complete. In fact all other tests work well without *2.

Also, there are improvements I would like to address / discuss. See PR reviews.

nighca · 2018-01-15T01:37:54Z

@wacky6 I can not find reviews or comments. Have you submitted them?

wacky6 · 2018-01-15T04:33:03Z

View on github. You should see comments in changes.

nighca · 2018-01-15T05:28:02Z

@wacky6 No comments in the Files Changed tab, either.

If there are reviews, they will appear among our conversations.

Leopoldthecoder · 2018-01-15T08:04:25Z

@wacky6 I couldn't find comments either. Could you have a check?

wacky6 · 2018-01-11T15:57:22Z

packages/date-picker/src/panel/date-range.vue

@@ -480,17 +497,24 @@
      },

      handleRangePick(val, close = true) {
-        if (this.maxDate === val.maxDate && this.minDate === val.minDate) {
+        console.log('range pick:', val.minDate, val.maxDate)


console.log删掉吧

wacky6 · 2018-01-11T16:01:35Z

examples/docs/en-US/date-picker.md

+        value11: '',
+        value12: '',
+        value13: '',
+        defaultTimeWithMin: [new Date(2018, 1, 1, 12)],


I prefer to write these in code, not in data()

How about only requiring time strings?

<el-date-picker type="datetimerange" :default-time="['08:00:00', '18:00:00']" />

It will be easier for users to provide time string instead of Date parsable object. But it will introduce a new format of time data and parse behavior, which means extra API & realization complexity. I'm not sure if it deserves that.

I found another problem of this PR: it is impossible to configure millisecond value of defaultTime now (because I'm using modifyTime). I will fix that.

I think fecha's parse method can handle time alone. (dateUtil in 'date-picker/util/index.js')
You can also refer to time-select panel's parseTime.

I don't think millisecond value is an issue since it is not respected in any picker panels (They may reset millisecond to 0).

Also I don't think there are compelling use cases for it. For example, using half-open intervals link [00:00:00.000, +1 days 00:00:00.000) instead of closed interval [00:00:00.000, 23:59:59.999] eliminates the need of millisecond values.

Thanks, I'll check it.

wacky6 · 2018-01-11T16:03:55Z

examples/docs/en-US/date-picker.md

@@ -394,6 +444,7 @@ This feature is at alpha stage. Feedback welcome.
 | picker-options | additional options, check the table below | object | — | {} |
 | range-separator | range separator | string | — | '-' |
 | default-value | optional, default date of the calendar | Date | anything accepted by `new Date()` | — |
+| default-time | optional, the time value to use when select datetime range in date table (type `datetimerange`) | Date[] | Array with length 1 or 2, each item accepted by `new Date()`. The first item for the start datetime and then second item for the end datetime | — |


How about ask for an array with length 2. e.g: `['08:00:00', null]

wacky6 · 2018-01-13T13:20:00Z

test/unit/specs/date-picker.spec.js

+
+      const vmWithDefaultTime = createTest(DatePicker, {
+        type: 'datetimerange',
+        value: [new Date(2000, 10, 10, 10, 10), new Date(2000, 10, 11, 10, 10)],


Use createVue. value is read-only in createTest, may result in failure to update internal states

wacky6 · 2018-01-15T08:14:50Z

Sorry. Seems I forgot to submit it.

nighca · 2018-01-15T14:28:50Z

@wacky6 Fixed.

wacky6 · 2018-01-15T17:34:56Z

LGTM, DEMO: https://jsfiddle.net/kxmnmLej/

TODOs:

How about "do not reset time if null/undefined" as mentioned in [Feature Request] option rightClosed for component date-picker with type datetimerange #7646 (comment)
(after merge) es doc

Leopoldthecoder · 2018-01-16T03:01:24Z

@nighca Thanks for contributing.

Actually default-time also affects pickers with type="daterange" (not only "on the date panel with type datetimerange"), which is a good thing, so after merging I'll update the docs to reflect this.

Are we good to merge this PR now? @wacky6

wacky6 · 2018-01-16T04:49:12Z

Yes, can merge.
Can finish Todos afterwards

Leopoldthecoder · 2018-01-16T12:39:42Z

@nighca This PR breaks the hovering background color when picking a range:

The correct behavior is:

Could you take another look?

nighca · 2018-01-16T13:25:08Z

@Leopoldthecoder Sure.

wacky6 · 2018-01-16T13:57:02Z

Should update tests to check picking hover behavior.

nighca · 2018-01-17T06:18:53Z

@Leopoldthecoder @wacky6 I created another PR to fix this, in which I also added some assertions to test this behavior. Plz review.

nighca force-pushed the date-picker-optimize branch from 859fd64 to e73aaac Compare January 8, 2018 05:42

nighca closed this Jan 8, 2018

nighca reopened this Jan 8, 2018

nighca force-pushed the date-picker-optimize branch 6 times, most recently from 816f192 to d10425a Compare January 11, 2018 16:13

DatePicker: add attribute defaultTime

d65f185

nighca force-pushed the date-picker-optimize branch from c2b4848 to d65f185 Compare January 11, 2018 16:31

wacky6 reviewed Jan 15, 2018

View reviewed changes

nighca added 2 commits January 15, 2018 17:22

use createVue instead of createTest & update docs for defaultTime

e77dbaf

update defaultTime type

676cf66

nighca force-pushed the date-picker-optimize branch from d1c4501 to 676cf66 Compare January 15, 2018 13:27

Leopoldthecoder approved these changes Jan 16, 2018

View reviewed changes

Leopoldthecoder merged commit 94f18ab into ElemeFE:dev Jan 16, 2018

nighca mentioned this pull request Jan 17, 2018

DatePicker: fix incorrect interaction on range change when defaultTime provided #9310

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DatePicker: add attribute defaultTime #9094

DatePicker: add attribute defaultTime #9094

nighca commented Jan 2, 2018 •

edited

Loading

nighca commented Jan 2, 2018

wacky6 commented Jan 2, 2018

wacky6 commented Jan 2, 2018

nighca commented Jan 2, 2018

nighca commented Jan 11, 2018

wacky6 commented Jan 13, 2018

nighca commented Jan 15, 2018

wacky6 commented Jan 15, 2018

nighca commented Jan 15, 2018 •

edited

Loading

Leopoldthecoder commented Jan 15, 2018

wacky6 Jan 11, 2018

wacky6 Jan 11, 2018 •

edited

Loading

nighca Jan 15, 2018

wacky6 Jan 15, 2018

wacky6 Jan 15, 2018

nighca Jan 15, 2018

wacky6 Jan 11, 2018 •

edited

Loading

nighca Jan 15, 2018

wacky6 Jan 13, 2018 •

edited

Loading

nighca Jan 15, 2018

wacky6 commented Jan 15, 2018

nighca commented Jan 15, 2018

wacky6 commented Jan 15, 2018

Leopoldthecoder commented Jan 16, 2018

wacky6 commented Jan 16, 2018

Leopoldthecoder commented Jan 16, 2018

nighca commented Jan 16, 2018

wacky6 commented Jan 16, 2018

nighca commented Jan 17, 2018 •

edited

Loading

DatePicker: add attribute defaultTime #9094

DatePicker: add attribute defaultTime #9094

Conversation

nighca commented Jan 2, 2018 • edited Loading

nighca commented Jan 2, 2018

wacky6 commented Jan 2, 2018

wacky6 commented Jan 2, 2018

nighca commented Jan 2, 2018

nighca commented Jan 11, 2018

wacky6 commented Jan 13, 2018

nighca commented Jan 15, 2018

wacky6 commented Jan 15, 2018

nighca commented Jan 15, 2018 • edited Loading

Leopoldthecoder commented Jan 15, 2018

wacky6 Jan 11, 2018

Choose a reason for hiding this comment

wacky6 Jan 11, 2018 • edited Loading

Choose a reason for hiding this comment

nighca Jan 15, 2018

Choose a reason for hiding this comment

wacky6 Jan 15, 2018

Choose a reason for hiding this comment

wacky6 Jan 15, 2018

Choose a reason for hiding this comment

nighca Jan 15, 2018

Choose a reason for hiding this comment

wacky6 Jan 11, 2018 • edited Loading

Choose a reason for hiding this comment

nighca Jan 15, 2018

Choose a reason for hiding this comment

wacky6 Jan 13, 2018 • edited Loading

Choose a reason for hiding this comment

nighca Jan 15, 2018

Choose a reason for hiding this comment

wacky6 commented Jan 15, 2018

nighca commented Jan 15, 2018

wacky6 commented Jan 15, 2018

Leopoldthecoder commented Jan 16, 2018

wacky6 commented Jan 16, 2018

Leopoldthecoder commented Jan 16, 2018

nighca commented Jan 16, 2018

wacky6 commented Jan 16, 2018

nighca commented Jan 17, 2018 • edited Loading

nighca commented Jan 2, 2018 •

edited

Loading

nighca commented Jan 15, 2018 •

edited

Loading

wacky6 Jan 11, 2018 •

edited

Loading

wacky6 Jan 11, 2018 •

edited

Loading

wacky6 Jan 13, 2018 •

edited

Loading

nighca commented Jan 17, 2018 •

edited

Loading