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

Select time feature #839

Merged
merged 18 commits into from Sep 12, 2017

Conversation

Projects
None yet
7 participants
@shadrech
Copy link
Contributor

shadrech commented Apr 23, 2017

Feature allows users to select a time along with the date. So moment object returned will have specific time
Also includes time filtering with minTime, maxTime and excludeTimes

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Apr 24, 2017

Codecov Report

Merging #839 into master will decrease coverage by 7.07%.
The diff coverage is 26.82%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #839      +/-   ##
==========================================
- Coverage   85.84%   78.77%   -7.08%     
==========================================
  Files          12       13       +1     
  Lines         643      716      +73     
  Branches      128      141      +13     
==========================================
+ Hits          552      564      +12     
- Misses          6       61      +55     
- Partials       85       91       +6
Impacted Files Coverage Δ
src/date_utils.js 79.66% <14.28%> (-20.34%) ⬇️
src/datepicker.jsx 89.94% <50%> (-2.15%) ⬇️
src/calendar.jsx 85.6% <63.15%> (-2.8%) ⬇️
src/time.jsx 9.09% <9.75%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e7b2fe...e400d89. Read the comment docs.

@shadrech

This comment has been minimized.

Copy link
Contributor

shadrech commented Apr 24, 2017

Will work on tests at the weekend. Will creating tests be all thats needed to fix the codecov errors?

@martijnrusschen

This comment has been minimized.

Copy link
Member

martijnrusschen commented Apr 24, 2017

@shadrech Thanks, yes. Codecov only complains about the fact that the current test coverage of this PR is below the target of 75%. Currently the overall coverage floats around 95% for the complete project.

@martijnrusschen
Copy link
Member

martijnrusschen left a comment

Based on the recent comment. Please add tests, to make sure all new functionality works as expected.

@@ -313,3 +313,30 @@ type: `string`
type: `bool`
defaultValue: `false`


This comment has been minimized.

@martijnrusschen

martijnrusschen Apr 24, 2017

Member

You can remove this submissions. This will be autogenerated when releasing a new version.

server.js Outdated
@@ -22,11 +22,11 @@ app.use(require('webpack-hot-middleware')(compiler))

app.use(express.static('docs-site'))

app.listen(8080, 'localhost', function (err) {
app.listen(9000, 'localhost', function (err) {

This comment has been minimized.

@martijnrusschen

martijnrusschen Apr 24, 2017

Member

Do we need this change?

This comment has been minimized.

@shadrech

shadrech Apr 24, 2017

Contributor

Sorry forgot to change that back. That port was occupied with a different process when I was developing the datepicker. That change isn't necessary at all

}

static get defaultProps () {
return {
dateFormat: 'L',
dateFormat: 'LLL',

This comment has been minimized.

@martijnrusschen

martijnrusschen Apr 24, 2017

Member

Why's this changed?

This comment has been minimized.

@shadrech

shadrech Apr 24, 2017

Contributor

"LLL" is more verbose. It displays the time as well. Maybe I should set a if statement if this.props.showTimeSelect is instantiated before changing the format to display time as well

@spiroski

This comment has been minimized.

Copy link

spiroski commented Jun 16, 2017

Any progress on this issue?

@shadrech

This comment has been minimized.

Copy link
Contributor

shadrech commented Jun 16, 2017

My bad. Been really busy at work. Will work on tests this weekend hopefully 🤞🏾

@shadrech shadrech force-pushed the shadrech:select-time-feature branch from 89bb96e to fe0a5b8 Sep 3, 2017

@shadrech

This comment has been minimized.

Copy link
Contributor

shadrech commented Sep 3, 2017

@martijnrusschen Sorry for hiatus. Added tests but codecov still not happy. Please advise on best way forward? What am I missing here?

@martijnrusschen
Copy link
Member

martijnrusschen left a comment

Looks cool! Few comments, I'll let some others review this as well since this is a pretty major addition to the component.

Also, perhaps it would be nice to add something to the Readme about this as well.

@@ -84,6 +84,8 @@
border-top-right-radius: 0.3rem;

This comment has been minimized.

@martijnrusschen

martijnrusschen Sep 4, 2017

Member

Can you please exclude changes to this file from the PR? This will be automatically updated when I'm going to run a new release.

@@ -1116,7 +1116,7 @@

This comment has been minimized.

@martijnrusschen

martijnrusschen Sep 4, 2017

Member

Can you please exclude changes to this file from the PR? This will be automatically updated when I'm going to run a new release.

@@ -68,4 +68,4 @@ General datepicker component.
|`value`|`string`|||

This comment has been minimized.

@martijnrusschen

martijnrusschen Sep 4, 2017

Member

Can you please exclude changes to this file from the PR? This will be automatically updated when I'm going to run a new release.

This comment has been minimized.

@shadrech

shadrech Sep 4, 2017

Contributor

I'm pretty sure I didn't edit these files. They must have been edited when running yarn start and editing files in src/ directory

This comment has been minimized.

@aij

aij Sep 5, 2017

Collaborator

@shadrech Yes, they're build outputs.

@martijnrusschen Is there a way we could improve the release process so everyone doesn't need to manually avoid committing these sorts of changes?

This comment has been minimized.

@shadrech

shadrech Sep 5, 2017

Contributor

@aij Add files ending in .js/.md/.css in directories docs/ & docs-site/ to .gitignore? Since files are auto generated

This comment has been minimized.

@martijnrusschen

martijnrusschen Sep 6, 2017

Member

Problem is that we need the .js and .css files in the gh-pages branch for the docs. It seems pretty hard to ignore something in one branch but have it checked in into the other branch. I read something about it here: https://stackoverflow.com/questions/1836742/using-git-how-do-i-ignore-a-file-in-one-branch-but-have-it-committed-in-another but haven't found the silver bullet yet. Ideas are welcome!

We can't ignore the .md files since those are served in the docs folder in the master branch.

@shadrech

This comment has been minimized.

Copy link
Contributor

shadrech commented Sep 4, 2017

Yh I can add this to README. I had just included examples of how to use the new feature in docs-site/src/examples (exclude_time_period.jsx, exclude_times.jsx, show_time.jsx). But yh I will add some info in README

@shadrech

This comment has been minimized.

Copy link
Contributor

shadrech commented Sep 4, 2017

What of the tests? @martijnrusschen

@martijnrusschen

This comment has been minimized.

Copy link
Member

martijnrusschen commented Sep 4, 2017

Not sure why codecov isn't liking them. Need to look into that, but for now this seems fine. @aij Any thougths on this PR?

@martijnrusschen

This comment has been minimized.

Copy link
Member

martijnrusschen commented Sep 4, 2017

@martijnrusschen martijnrusschen requested review from alex-shamshurin and aij Sep 4, 2017

@aij
Copy link
Collaborator

aij left a comment

Thanks for the great work.

@@ -264,7 +281,7 @@ export default class Calendar extends React.Component {
var monthDate = this.state.date.clone().add(i, 'M')
var monthKey = `month-${i}`
monthList.push(
<div key={monthKey} className="react-datepicker__month-container">
<div key={monthKey} ref="monthContainer" className="react-datepicker__month-container">

This comment has been minimized.

@aij

aij Sep 5, 2017

Collaborator

ref="string" is deprecated in newer React. The new equivalent is ref={r => this.refs.monthContainer = r} instead, though most people seem to be taking it a step further by avoiding this.refs entirely, though I'm not sure there's any basis for the later beyond a prominent example.


export function isTimeInDisabledRange (time, { minTime, maxTime }) {
if (!minTime || !maxTime) {
throw new Error('Both minTime and maxTime props required')

This comment has been minimized.

@aij

aij Sep 5, 2017

Collaborator

Why should both be required? For dates we allow upper and lower bounds to be specified independently.

This comment has been minimized.

@shadrech

shadrech Sep 5, 2017

Contributor

Only applicable if user decides to have a time range. Both values required to create time window. Since time loops at 24 (24 hr clock), setting a lower bound with no upper bound for example won't create a finite window (not good at explaining, hopefully that makes sense)

@@ -270,6 +277,20 @@ export default class DatePicker extends React.Component {
}
}

handleTimeChange = (time) => {
const selected = (this.props.selected) ? this.props.selected : moment()

This comment has been minimized.

@aij

aij Sep 5, 2017

Collaborator

We don't always default to the current date, and it is not always valid. See calcInitialState()/ setOpen().

import React from 'react'
import PropTypes from 'prop-types'
import ReactDOM from 'react-dom'
import Tether from 'tether'

This comment has been minimized.

@aij

aij Sep 5, 2017

Collaborator

This looks like the file that was deleted with the conversion to Popper. Was it accidentally reverted somehow?

This comment has been minimized.

@martijnrusschen

martijnrusschen Sep 6, 2017

Member

Yeah seems an artifact of recent changes. We don't use Tether anymore.

static propTypes = {
intervals: PropTypes.number,
selected: PropTypes.object,
onTimeChange: PropTypes.func,

This comment has been minimized.

@aij

aij Sep 5, 2017

Collaborator

I have a slight preference for standard prop names (eg onChange) if we expect this component to be used directly, so users don't need to wrap the component in another component that does little more than renaming props.

selected={this.state.startDate}
onChange={this.handleChange}
showTimeSelect
excludeTimes={[moment().hours(17).minutes(0), moment().hours(18).minutes(30), moment().hours(19).minutes(30), moment().hours(17).minutes(30)]}

This comment has been minimized.

@aij

aij Sep 5, 2017

Collaborator

Making this a function/predicate instead of an array might be more flexible, especially if whole time ranges need to be excluded, or if the exclusions need to depend on the date. (Eg, if it is based on available appointment times from a DB, or varies with DST.)

If we expect a fixed/predefined list to be much more common this could be easier to use though. Thoughts?

This comment has been minimized.

@shadrech

shadrech Sep 10, 2017

Contributor

I find an array to be more simpler. I imagine a scenario where an api call returns a json array of moment.toISOString strings, and all client has to do is convert these to moment() objects and plug the array into excludeTimes. Even maybe extend excludeTime that first a check is made to see if objects received are strings, and automatically convert to moment() objects. That way client can just pass the json array directly to excludeTimes via a simple JSON.parse?

.react-datepicker__time-container .react-datepicker__time {
position: relative;
background: white; }
.react-datepicker__time-container .react-datepicker__time .react-datepicker__time-box {

This comment has been minimized.

@aij

aij Sep 5, 2017

Collaborator

Indentation here and below.

Also seems like a lot of special casing.

This comment has been minimized.

@martijnrusschen

martijnrusschen Sep 6, 2017

Member

Auto generated file, should be removed from this PR.

This comment has been minimized.

@shadrech

shadrech Sep 10, 2017

Contributor

Yh most of it removed. Main function of special casing was for sizing the time component. Now thats done by calculating the month container height via clientHeight (passed via refs)

@@ -68,4 +68,4 @@ General datepicker component.
|`value`|`string`|||

This comment has been minimized.

@aij

aij Sep 5, 2017

Collaborator

@shadrech Yes, they're build outputs.

@martijnrusschen Is there a way we could improve the release process so everyone doesn't need to manually avoid committing these sorts of changes?

@@ -193,8 +195,16 @@ export default class exampleComponents extends React.Component {
component: <RawChange/>
},
{
title: 'Don\'t hide calendar on date selection',
component: <DontCloseOnSelect/>

This comment has been minimized.

@aij

aij Sep 5, 2017

Collaborator

Why was this example removed?

maxTime={moment().hours(18).minutes(0)} />
)
var times = datePicker.find('li.react-datepicker__time-list-item')
expect(times).to.exist

This comment has been minimized.

@aij

aij Sep 5, 2017

Collaborator

Wouldn't this test pass even if minTime and maxTime are ignored?

This comment has been minimized.

@shadrech

shadrech Sep 10, 2017

Contributor

Lol yh my bad. At first I wanted to test the number of .react-datepicker__time-list-item they'll be; there should be 3 with configuration above. I tried testing with expect(times).to.have.length(3) but kept getting a fail as length always comes back as 0. Any ideas what I'm missing here? Maybe theres another test I can do that'll achieve desired result

@shadrech

This comment has been minimized.

Copy link
Contributor

shadrech commented Sep 6, 2017

Will work on requests at the weekend

Tatenda Chawanzwa added some commits Sep 10, 2017

Tatenda Chawanzwa
Code review changes
 - Removed excessive special casing in scss

 - Reinstated DontCloseOnSelect

 - refactored onTimeChange prop name to onChange on Time.tsx

 - Moved time examples to top of example_components
Tatenda Chawanzwa
@@ -0,0 +1,149 @@
import React from 'react'

This comment has been minimized.

@martijnrusschen

martijnrusschen Sep 11, 2017

Member

Please remove the entire file. This isn't used.

This comment has been minimized.

@shadrech

shadrech Sep 12, 2017

Contributor

👍

@martijnrusschen martijnrusschen merged commit 1957c31 into Hacker0x01:master Sep 12, 2017

1 of 3 checks passed

codecov/patch 26.82% of diff hit (target 85.84%)
Details
codecov/project 78.77% (-7.08%) compared to 7e7b2fe
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dola

This comment has been minimized.

Copy link

dola commented Sep 19, 2017

Overall looking pretty cool. I'm looking forward to using this!
However I would advise to only merge to master when the feature is due to be released. Otherwise the master Readme shows documentation for a feature that is not yet available. (confuses new users)

@actraiser

This comment has been minimized.

Copy link

actraiser commented Sep 20, 2017

Thanks @dola - that was exactly the case for me. I was wondering why adding showTimeSelect to the props did not show up anything when I tested react-datepicker for the first time using 0.54.0.

@actraiser

This comment has been minimized.

Copy link

actraiser commented Sep 20, 2017

PS: The just released 0.55.0 now behaves as expected. Thx!

@martijnrusschen

This comment has been minimized.

Copy link
Member

martijnrusschen commented Sep 20, 2017

Sorry for the confusion, forgot to release this earlier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment