New props, functionalities and behaviour #18

Serg4554 · 2018-08-20T12:05:23Z

Hello!
I needed some extra functionalities for my project and I thought that it could be useful for others, so here is the pull request :)

minutesStep property (number)
Allows to choose the steps between minutes, so for instance, if you need to allow times only each 5 minutes, just set minutesStep={5}
cancelOnClose property (bool)
Allows to decide if the time is cancelled or saved when background is clicked
inputClasses property (object)
Allows to apply classes to input form (only TimePicker was supported)
Calling onChange when time changes
The time is updated (onChange is called) whenever the time is changed and return the init date when is cancelled

I hope you find it useful 😃👍

Serg4554 · 2018-08-20T14:40:45Z

src/TimePicker.spec.js

@@ -78,7 +78,6 @@ describe('<TimePicker />', () => {
        .simulate('click', testUtils.stubClickEvent(190, 230)) // click on 25
        .simulate('mouseup', testUtils.stubClickEvent(190, 230)) // click on 25

-      expect(changeHandler).toBeCalled()


Removed by mistake, amended in the next commit

Serg4554 · 2018-08-20T14:40:54Z

src/TimePicker.spec.js

@@ -260,7 +259,6 @@ describe('<TimePicker />', () => {
        .simulate('click', testUtils.stubClickEvent(190, 230)) // click on 25
        .simulate('mouseup', testUtils.stubClickEvent(190, 230)) // click on 25

-      expect(changeHandler).toBeCalled()


Removed by mistake, amended in the next commit

Serg4554 · 2018-08-20T14:42:37Z

src/TimeInput.spec.js

@@ -141,7 +141,6 @@ describe('<TimeInput />', () => {
      tree.find(Button).at(0).simulate('click')

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


Now is called in every date change and restored when canceled

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

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.

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

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

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

coveralls · 2018-08-20T16:59:13Z

Coverage decreased (-2.9%) to 93.286% when pulling bb09972 on Serg4554:minutes-steps-and-onchange-update into 79b29df on TeamWertarbyte:master.

leMaik · 2018-09-13T18:55:12Z

@Serg4554 I'm sorry I didn't see this earlier! This is an awesome first contribution, thank you so much! 🎉

I'd rename cancelOnClose to selectOnClose. 🤔 What do you think?

Serg4554 · 2018-09-13T19:04:07Z

Thanks to you for this awesome component!
About the property rename, yes, no problem, I agree 👍

leMaik

Thanks again for this great PR! ❤️ Some nit-picky things (don't worry about them too much 😉).

I don't like the idea of calling onChange even if there are no changes. Is there an actual use case where this behavior is needed? 🤔

Regarding test coverage: While I aim at 💯 %, the -1,2% is fine. 👍

leMaik · 2018-09-13T18:58:14Z

src/TimePicker.js

-      this.setState({ minutes: value }, () => {
-        this.propagateChange()
-      })
+    } else if (value % (this.props.minutesStep || 1) === 0) {


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

leMaik · 2018-09-13T18:58:23Z

src/TimePicker.js

@@ -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}


leMaik · 2018-09-13T19:00:49Z

src/Clock.js

+              className={classNames(
+                classes.number,
+                { selected: value === digit.display || (digit.display === 60 && value === 0) },
+                { disabled: digit.display % (this.props.minutesStep || 1) })}


These two objects can be merged.

leMaik · 2018-09-13T19:01:07Z

src/Clock.js

-  value: PropTypes.number.isRequired
+  value: PropTypes.number.isRequired,
+  /** Steps between minutes. */
+  minutesStep: PropTypes.number


The propTypes were sorted alphabetically before. 😉

leMaik · 2018-09-13T19:02:55Z

src/TimeInput.js

@@ -80,19 +89,21 @@ class TimeInput extends React.Component {
        value={formattedValue}
        readOnly
        key='TimeInput-input'
+        classes={inputClasses || {}}


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

leMaik · 2018-09-13T19:03:37Z

src/TimeInput.js

          onChange={this.handleChange}
          onMinutesSelected={autoOk ? this.handleOk : null}
          classes={{ header: classes.header, body: classes.body }}
+          minutesStep={minutesStep || 1}


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

leMaik · 2018-09-13T19:03:56Z

src/TimeInput.js

@@ -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),


These used to be sorted alphabetically. 😅

leMaik · 2018-09-13T19:04:06Z

src/TimeInput.js

 }

 TimeInput.defaultProps = {
  autoOk: false,
  cancelLabel: 'Cancel',
  mode: '12h',
-  okLabel: 'Ok'
+  okLabel: 'Ok',


…these, too

leMaik · 2018-09-13T19:04:20Z

src/TimePicker.js

-  value: PropTypes.instanceOf(Date)
+  value: PropTypes.instanceOf(Date),
+  /** Steps between minutes. */
+  minutesStep: PropTypes.number


Serg4554 · 2018-09-13T19:17:56Z

Than you very much for taking the time to review all changes. Right now I'm developing a project against the clock 😅, but as soon as I finish I will try to leave it perfect 👌

leMaik · 2018-09-13T19:46:12Z

@Serg4554 No problem, I'm happy that you'll take the time to get this PR ready to merge. :)

leMaik · 2018-09-24T09:13:20Z

@Serg4554 Do you still have time to finish this PR? Don't worry, I'll update it for you otherwise. 👍

Serg4554 · 2018-09-24T10:45:56Z

@leMaik I will finish the current project that I'm working on in a month aprox. but we have very tight time, so if you could update it, I would appreciate it! Otherwise I will do it, you have my words

Serg4554 · 2018-10-12T17:21:59Z

@leMaik All changes we discussed are done, I hope you find them fine and useful! But the coverage has decreased a little bit, I will thank you if you could help me with that :)

leMaik · 2018-10-29T22:31:18Z

@Serg4554 Awesome! 🎉 I've got this on my radar, I'll look into the tests soon (TM).

leMaik · 2018-11-03T19:00:05Z

@Serg4554 Would you mind rebasing this on top of master? I'm a bit confused by all the changes right now…

Serg4554 added 5 commits August 20, 2018 13:49

Added minutesSteps, cancelOnClose and calling onChange when time changes

be91fd6

Fixed code style

9683c74

Bugs in uncontrolled mode fixed

49cb3ff

Updated snapshots and test specs

aabcbaf

Ammend last removed line

1e069f5

Serg4554 commented Aug 20, 2018

View reviewed changes

inputClasses property added

c513fc1

Serg4554 changed the title ~~Added minutesSteps, cancelOnClose and calling onChange when time changes~~ New props, functionalities and behaviour Aug 20, 2018

Updated snapshot

3f2953f

Disabled numbers with less opacity (disabled class)

2add890

leMaik added the enhancement label Sep 13, 2018

leMaik requested changes Sep 13, 2018

View reviewed changes

Serg4554 added 7 commits October 12, 2018 17:39

Code cleaned

b66f299

Added updateImmediately property

dfe2d86

Set initValue when value prop is updated

4c9eb3a

Updated snapshots

256e1ab

Renamed property: cancelOnClose to selectOnClose

7328a86

Props alphabetically sorted

8aba6e0

Standard style

bb09972

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New props, functionalities and behaviour #18

New props, functionalities and behaviour #18

Serg4554 commented Aug 20, 2018 •

edited

Loading

Serg4554 Aug 20, 2018

Serg4554 Aug 20, 2018

Serg4554 Aug 20, 2018

leMaik Sep 13, 2018

Serg4554 Sep 13, 2018

leMaik Sep 13, 2018 •

edited

Loading

Serg4554 Sep 13, 2018

leMaik Sep 13, 2018

coveralls commented Aug 20, 2018 •

edited

Loading

leMaik commented Sep 13, 2018 •

edited

Loading

Serg4554 commented Sep 13, 2018

leMaik left a comment •

edited

Loading

leMaik Sep 13, 2018

leMaik Sep 13, 2018

leMaik Sep 13, 2018

leMaik Sep 13, 2018

leMaik Sep 13, 2018

leMaik Sep 13, 2018

leMaik Sep 13, 2018

leMaik Sep 13, 2018

leMaik Sep 13, 2018

Serg4554 commented Sep 13, 2018

leMaik commented Sep 13, 2018

leMaik commented Sep 24, 2018

Serg4554 commented Sep 24, 2018

Serg4554 commented Oct 12, 2018

leMaik commented Oct 29, 2018

leMaik commented Nov 3, 2018

New props, functionalities and behaviour #18

Are you sure you want to change the base?

New props, functionalities and behaviour #18

Conversation

Serg4554 commented Aug 20, 2018 • edited Loading

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

Choose a reason for hiding this comment

leMaik Sep 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 20, 2018 • edited Loading

leMaik commented Sep 13, 2018 • edited Loading

Serg4554 commented Sep 13, 2018

leMaik left a comment • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Serg4554 commented Sep 13, 2018

leMaik commented Sep 13, 2018

leMaik commented Sep 24, 2018

Serg4554 commented Sep 24, 2018

Serg4554 commented Oct 12, 2018

leMaik commented Oct 29, 2018

leMaik commented Nov 3, 2018

Serg4554 commented Aug 20, 2018 •

edited

Loading

leMaik Sep 13, 2018 •

edited

Loading

coveralls commented Aug 20, 2018 •

edited

Loading

leMaik commented Sep 13, 2018 •

edited

Loading

leMaik left a comment •

edited

Loading