Skip to content
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

New props, functionalities and behaviour #18

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
14 changes: 10 additions & 4 deletions src/Clock.js
100644 → 100755
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ const styles = (theme) => ({
userSelect: 'none',
'&.selected': {
color: getContrastRatio(theme.palette.primary.main, theme.palette.common.black) < 7 ? theme.palette.common.white : theme.palette.common.black
}
},
'&.disabled': { opacity: '0.3' }
},
smallNumber: {
fontSize: '12px',
Expand Down Expand Up @@ -134,7 +135,7 @@ class Clock extends React.PureComponent {
}

render () {
const { classes, mode, value, ...other } = this.props
const { classes, mode, value, minutesStep, ...other } = this.props
const { touching } = this.state

return (
Expand Down Expand Up @@ -191,7 +192,10 @@ class Clock extends React.PureComponent {
{mode === 'minutes' && getNumbers(12, { size, start: 5, step: 5 }).map((digit, i) => (
<span
key={digit.display}
className={classNames(classes.number, { selected: value === digit.display || (digit.display === 60 && value === 0) })}
className={classNames(
classes.number,
{ selected: value === digit.display || (digit.display === 60 && value === 0) },
{ disabled: digit.display % (this.props.minutesStep || 1) })}
Copy link
Member

Choose a reason for hiding this comment

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

These two objects can be merged.

style={{
transform: `translate(${digit.translateX}px, ${digit.translateY}px)`
}}
Expand All @@ -211,7 +215,9 @@ Clock.propTypes = {
/** Callback that is called with the new hours/minutes (as a number) when the value is changed. */
onChange: PropTypes.func,
/** The value of the clock. */
value: PropTypes.number.isRequired
value: PropTypes.number.isRequired,
/** Steps between minutes. */
minutesStep: PropTypes.number
Copy link
Member

Choose a reason for hiding this comment

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

The propTypes were sorted alphabetically before. 😉

}

export default withStyles(styles)(Clock)
Expand Down
45 changes: 32 additions & 13 deletions src/TimeInput.js
100644 → 100755
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ class TimeInput extends React.Component {
const defaultValue = new Date()
defaultValue.setSeconds(0)
defaultValue.setMilliseconds(0)
this.state = { open: false, value: props.value || props.defaultValue || defaultValue }
const initValue = props.value || props.defaultValue || defaultValue
this.state = { open: false, initValue, value: initValue }
}

componentWillReceiveProps (nextProps) {
Expand All @@ -33,36 +34,44 @@ class TimeInput extends React.Component {
}
}

showDialog = () => this.setState({ open: true, newValue: this.state.value })
showDialog = () => this.setState({ open: true })

handleChange = (newValue) => {
this.setState({ newValue })
handleChange = (value) => {
if (this.props.onChange != null) {
this.props.onChange(value)
}
this.setState({ value })
}

handleOk = () => {
this.setState({ open: false, initValue: this.state.value })
}

handleCancel = () => {
if (this.props.onChange != null) {
this.props.onChange(this.state.newValue)
this.props.onChange(this.state.initValue)
}
this.setState({ open: false, value: this.state.newValue, newValue: null })
this.setState({ open: false, value: this.state.initValue })
}

handleCancel = () => this.setState({ open: false, newValue: null })

render () {
const {
autoOk,
cancelLabel,
classes,
inputClasses,
defaultValue, // eslint-disable-line
disabled: disabledProp,
mode,
okLabel,
onChange, // eslint-disable-line
value: valueProp, // eslint-disable-line
minutesStep,
cancelOnClose,
...other
} = this.props

const { value, newValue } = this.state
const { value } = this.state

const { hours, isPm } = formatHours(value.getHours(), mode)
const formattedValue = mode === '12h'
Expand All @@ -80,19 +89,21 @@ class TimeInput extends React.Component {
value={formattedValue}
readOnly
key='TimeInput-input'
classes={inputClasses || {}}
Copy link
Member

Choose a reason for hiding this comment

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

No reason to set this to an empty object. Also, instead of introducing a new prop, this could as well be classes.input (just add input: {} to the styles object above, the user will then be able to customize it with classes={{ input: 'the-users-input-class' }}, a common Material-UI pattern).

/>,
<Dialog
maxWidth='xs'
open={this.state.open}
key='TimeInput-dialog'
onClose={this.handleCancel}
onClose={cancelOnClose ? this.handleCancel : this.handleOk}
>
<TimePicker
mode={mode}
value={newValue}
value={value}
onChange={this.handleChange}
onMinutesSelected={autoOk ? this.handleOk : null}
classes={{ header: classes.header, body: classes.body }}
minutesStep={minutesStep || 1}
Copy link
Member

Choose a reason for hiding this comment

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

minutesStep has a default value set below, so || 1 can be dropped here

/>
<DialogActions>
<Button onClick={this.handleCancel} color='primary'>{cancelLabel}</Button>
Expand All @@ -117,14 +128,22 @@ TimeInput.propTypes = {
/** Callback that is called with the new date (as Date instance) when the value is changed. */
onChange: PropTypes.func,
/** The value of the time picker, for use in controlled mode. */
value: PropTypes.instanceOf(Date)
value: PropTypes.instanceOf(Date),
Copy link
Member

Choose a reason for hiding this comment

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

These used to be sorted alphabetically. 😅

/** Steps between minutes. */
minutesStep: PropTypes.number,
/** Returns init date when dialog is closed (clicking background). */
cancelOnClose: PropTypes.bool,
/** Classes applied to input. */
inputClasses: PropTypes.object
}

TimeInput.defaultProps = {
autoOk: false,
cancelLabel: 'Cancel',
mode: '12h',
okLabel: 'Ok'
okLabel: 'Ok',
Copy link
Member

Choose a reason for hiding this comment

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

…these, too

minutesStep: 1,
cancelOnClose: true
}

TimeInput.contextTypes = {
Expand Down
1 change: 0 additions & 1 deletion src/TimeInput.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,6 @@ describe('<TimeInput />', () => {
tree.find(Button).at(0).simulate('click')

expect(getValue(tree)).toBe('13:37') // unchanged
expect(changeHandler).not.toHaveBeenCalled()
Copy link
Author

Choose a reason for hiding this comment

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

Now is called in every date change and restored when canceled

Copy link
Member

Choose a reason for hiding this comment

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

Why? There is no reason to call the change handler when the date isn't changed.

Copy link
Author

Choose a reason for hiding this comment

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

I did that change because I needed the time text field (and some other components) to be updated when the user changes the hour in real time, so every time the user moves the clock hands, changeHandler is called.

Copy link
Member

@leMaik leMaik Sep 13, 2018

Choose a reason for hiding this comment

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

I see, that's a good point. 🤔 We should make that optional and turn it off by default. Maybe call that prop updateImmediately?

Copy link
Author

Choose a reason for hiding this comment

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

In fact, I think that's the best option, thus maintaining the same behavior (avoiding some developers from going crazy when updating 🙃) and providing that functionality, which in my case is very useful

Copy link
Member

Choose a reason for hiding this comment

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

That was my intention. 👍 Adding your use case while keeping this a minor update (i.e. no breaking changes).

})
})
})
Expand Down
11 changes: 6 additions & 5 deletions src/TimePicker.js
100644 → 100755
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,8 @@ class TimePicker extends React.Component {
} else {
this.setState({ hours: value }, this.propagateChange)
}
} else {
this.setState({ minutes: value }, () => {
this.propagateChange()
})
} else if (value % (this.props.minutesStep || 1) === 0) {
Copy link
Member

Choose a reason for hiding this comment

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

The default step should better be set in the default props.

this.setState({ minutes: value }, this.propagateChange)
}
}

Expand Down Expand Up @@ -186,6 +184,7 @@ class TimePicker extends React.Component {
value={clockMode === 'minutes' ? minutes : hours}
onMouseUp={this.handleClockChangeDone}
onTouchEnd={this.handleClockChangeDone}
minutesStep={this.props.minutesStep || 1}
Copy link
Member

Choose a reason for hiding this comment

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

See above.

/>
</div>
</div>
Expand All @@ -203,7 +202,9 @@ TimePicker.propTypes = {
/** Callback that is called when the minutes are changed. Can be used to automatically hide the picker after selecting a time. */
onMinutesSelected: PropTypes.func,
/** The value of the time picker, for use in controlled mode. */
value: PropTypes.instanceOf(Date)
value: PropTypes.instanceOf(Date),
/** Steps between minutes. */
minutesStep: PropTypes.number
Copy link
Member

Choose a reason for hiding this comment

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

see above

}

TimePicker.defaultProps = {
Expand Down
6 changes: 6 additions & 0 deletions src/__snapshots__/TimeInput.spec.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,20 @@ exports[`<TimeInput /> 12h matches the snapshot 1`] = `
<TimeInput
autoOk={false}
cancelLabel="Cancel"
cancelOnClose={true}
classes={
Object {
"body": "TimeInput-body-2",
"header": "TimeInput-header-1",
}
}
defaultValue={2017-10-15T13:37:00.000Z}
minutesStep={1}
mode="12h"
okLabel="Ok"
>
<WithStyles(Input)
classes={Object {}}
disabled={false}
key="TimeInput-input"
onClick={[Function]}
Expand Down Expand Up @@ -170,17 +173,20 @@ exports[`<TimeInput /> 24h matches the snapshot 1`] = `
<TimeInput
autoOk={false}
cancelLabel="Cancel"
cancelOnClose={true}
classes={
Object {
"body": "TimeInput-body-2",
"header": "TimeInput-header-1",
}
}
defaultValue={2017-10-15T13:37:00.000Z}
minutesStep={1}
mode="24h"
okLabel="Ok"
>
<WithStyles(Input)
classes={Object {}}
disabled={false}
key="TimeInput-input"
onClick={[Function]}
Expand Down
4 changes: 4 additions & 0 deletions src/__snapshots__/TimePicker.spec.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ exports[`<TimePicker /> 12h matches the snapshot 1`] = `
className="TimePicker-body-7"
>
<WithStyles(Clock)
minutesStep={1}
mode="12h"
onChange={[Function]}
onMouseUp={[Function]}
Expand All @@ -86,6 +87,7 @@ exports[`<TimePicker /> 12h matches the snapshot 1`] = `
"smallPointer": "Clock-smallPointer-14",
}
}
minutesStep={1}
mode="12h"
onChange={[Function]}
onMouseUp={[Function]}
Expand Down Expand Up @@ -317,6 +319,7 @@ exports[`<TimePicker /> 24h matches the snapshot 1`] = `
className="TimePicker-body-7"
>
<WithStyles(Clock)
minutesStep={1}
mode="24h"
onChange={[Function]}
onMouseUp={[Function]}
Expand All @@ -338,6 +341,7 @@ exports[`<TimePicker /> 24h matches the snapshot 1`] = `
"smallPointer": "Clock-smallPointer-14",
}
}
minutesStep={1}
mode="24h"
onChange={[Function]}
onMouseUp={[Function]}
Expand Down