[TimePicker] Update time state on new defaultTime prop. #3095

rscnt · 2016-01-29T23:45:11Z

This pull request try to fix #3094.

If a TimePicker instance is already mounted and a new defaultTime is passed to it, the time state value doesn't update.

There was already a pull request aiming to fix this and other issues for this component at #2027, but it has been waiting for review since Nov 24, 2015.

oliviertassinari · 2016-01-29T23:56:06Z

src/time-picker/time-picker.jsx

+      newState.muiTheme = nextContext.muiTheme; 
+    }
+    if (!Date.isEqualTime(this.state.time, nextProps.defaultTime)) {
+      newState.time = nextProps.defaultTime;


I think that the defaultTime should only be used during the first render, like for a regular <input />. I would add a new value property for your "controlled" use case.

@oliviertassinari of course, that would be better, the prop value is going to have precedence over defaultTime at first rendering, right?

mbrookes · 2016-01-30T00:56:02Z

it has been waiting for review since Nov 24, 2015

Ah, technically it has been waiting for revision since then. Happens frequently. I call them drive by PRs! :D

Anyway, back to business! DatePicker has a value prop and a controlled example, you could model TimePicker off that.

rscnt · 2016-01-31T00:08:23Z

@mbrookes, @oliviertassinari

The TimePicker now adopts a behaviour similar to the DatePicker one.

Tests have been added to prove the fix, although they don't cover all scenarios, it show at least that the change is working 😅

mbrookes · 2016-01-31T00:43:41Z

Gonna leave @oliviertassinari for the final review, but have to say, awesome work!

oliviertassinari · 2016-01-31T10:04:33Z

@rscnt Wow, thanks for the tests! Could you update the examples of the doc? I would be better to remove the warnings from the console (linked to the deprecated methods)

oliviertassinari · 2016-01-31T10:06:25Z

src/time-picker/time-picker.jsx

@@ -80,6 +82,11 @@ const TimePicker = React.createClass({
    textFieldStyle: React.PropTypes.object,

    /**
+     * Sets the time for the Time Picker programmatically


missing dot at the end of the sentence.

rscnt · 2016-01-31T15:10:38Z

@oliviertassinari docs have been updated, commentaries have been applied and mistakes have been fixed 😄

oliviertassinari · 2016-01-31T15:43:40Z

docs/src/app/components/pages/components/TimePicker/ExampleComplex.jsx

+    super(props);
+    this.state = {value24: new Date(), value12: new Date()};
+    this.handleChangeTimePicker24 = this.handleChangeTimePicker24.bind(this);
+    this.handleChangeTimePicker12 = this.handleChangeTimePicker12.bind(this);


Any reason for using bind over the arrow function property?

rscnt · 2016-01-31T19:19:23Z

@oliviertassinari warnings added and TimePicker/ComplexExample is now using arrow syntax.

oliviertassinari · 2016-01-31T19:30:53Z

@rscnt Thanks! I will have a closer look at this PR, but that look good so far. I have seen some minor issue, like using let where const would work or adding a _ for method, but I wouldn't worry too much about it.
By the way, before we merge, you will have to squash down the commits. (make the history cleaner).

rscnt · 2016-02-01T13:03:21Z

@oliviertassinari 'squashed' 👍

oliviertassinari · 2016-02-01T17:46:00Z

docs/src/app/components/pages/components/TimePicker/ExampleComplex.jsx

@@ -3,13 +3,17 @@ import TimePicker from 'material-ui/lib/time-picker/time-picker';

 export default class TimePickerExampleComplex extends React.Component {

+  constructor(props) {
+    super(props);
+    this.state = {value24: new Date(), value12: new Date()};


Could we have only one state for this example with a default value null as before.
It would be something like:

this.state = { value: null, };

oliviertassinari · 2016-02-01T18:06:36Z

@rscnt We are almost there :).

mbrookes · 2016-02-11T22:04:32Z

Hah, I was using this PR to test error output on gpr, and the errors went away. 😆

oliviertassinari · 2016-02-12T12:04:55Z

src/time-picker/time-picker.jsx

@@ -204,7 +228,7 @@ const TimePicker = React.createClass({
          {...other}
          style={textFieldStyle}
          ref="input"
-          value={time === emptyTime ? null : this.formatTime(time)}
+          value={time === emptyTime ? null : DateTime.formatTime(time, this.props.format, this.props.pedantic)}


What about DateTime.formatTime(time, format, pedantic)?

oliviertassinari · 2016-02-12T12:12:37Z

@rscnt Thanks!
Sorry, that's not ready to be merged 😁. I have more feedbacks and we have two conflicts with the master branch (wording and WindowsListener).

oliviertassinari · 2016-02-12T20:28:07Z

src/utils/date-time.js

@@ -165,4 +223,12 @@ export default {
    return ~~(this.monthDiff(d1, d2) / 12);
  },

+  isEqualTime(d1, d2) {


Any reason for keeping this method? That's no longer used.

rscnt · 2016-02-14T18:32:18Z

@oliviertassinari removed.

oliviertassinari · 2016-02-14T18:41:59Z

docs/src/app/components/pages/components/TimePicker/ExampleComplex.jsx

-          ref="picker24hr"
-          format="24hr"
-          hintText="24hr Format"
+        <TimePicker format="ampm" hintText="12hr Format"


Could you follow this coding style?

<TimePicker format="ampm" hintText="12hr Format"

oliviertassinari · 2016-02-14T18:44:16Z

@rscnt Thanks! I have done a final review of this PR, aside from my two comments and the Travis fail. I think that we can merge. Good job 🎉.

src/utils/date-time.js
  159:11  error  `isAM` is never modified, use `const` instead        prefer-const
  161:11  error  `additional` is never modified, use `const` instead  prefer-const

alitaheri · 2016-02-14T19:04:57Z

This looks awesome 😍 can't wait to see it merged 👍

- TimePicker#getValue is now deprecated. - TimePicker#setValue is now deprecated. - Added value prop. - defaultTime is only used if value is not defined on props. - Added tests for TimePicker initialization behavior. - Adding utils/DateTime#formatTime Fixes mui#3094 Signed-off-by: rscnt <[email protected]>

rscnt · 2016-02-15T17:44:34Z

@oliviertassinari sorry, linting issues have been fixed.

[TimePicker] Update time state on new defaultTime prop.

oliviertassinari · 2016-02-15T17:52:10Z

@rscnt Thanks that's a great feature 🚀.

oliviertassinari reviewed Jan 29, 2016
View reviewed changes

oliviertassinari added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Jan 30, 2016

mbrookes added PR: Needs Review and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Jan 31, 2016

oliviertassinari reviewed Jan 31, 2016
View reviewed changes

rscnt force-pushed the master branch from f4cb938 to 0641e64 Compare February 1, 2016 12:55

oliviertassinari reviewed Feb 1, 2016
View reviewed changes

oliviertassinari added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Feb 1, 2016

rscnt mentioned this pull request Feb 11, 2016

[TimePicker] state.time doesn't updates on new defaultTime prop. #3094

Closed

rscnt force-pushed the master branch 2 times, most recently from 1dcbd2f to e180bc3 Compare February 11, 2016 21:56

mbrookes added PR: Needs Review and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Feb 11, 2016

oliviertassinari reviewed Feb 12, 2016
View reviewed changes

rscnt force-pushed the master branch 2 times, most recently from 6c5798a to 2b41865 Compare February 12, 2016 19:39

oliviertassinari reviewed Feb 12, 2016
View reviewed changes

rscnt force-pushed the master branch from c9b836a to 2117dca Compare February 14, 2016 18:31

oliviertassinari reviewed Feb 14, 2016
View reviewed changes

rscnt force-pushed the master branch 2 times, most recently from 59f76b9 to b6e0271 Compare February 15, 2016 16:43

rscnt force-pushed the master branch from b6e0271 to 1345af8 Compare February 15, 2016 16:50

oliviertassinari added PR: Review Accepted and removed PR: Needs Review labels Feb 15, 2016

oliviertassinari added a commit that referenced this pull request Feb 15, 2016

Merge pull request #3095 from rscnt/master

7df55c9

[TimePicker] Update time state on new defaultTime prop.

oliviertassinari merged commit 7df55c9 into mui:master Feb 15, 2016

zannager added component: time picker This is the name of the generic UI component, not the React module! component: pickers This is the name of the generic UI component, not the React module! labels Mar 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TimePicker] Update time state on new defaultTime prop. #3095

[TimePicker] Update time state on new defaultTime prop. #3095

rscnt commented Jan 29, 2016

oliviertassinari Jan 29, 2016

rscnt Jan 30, 2016

mbrookes commented Jan 30, 2016

rscnt commented Jan 31, 2016

mbrookes commented Jan 31, 2016

oliviertassinari commented Jan 31, 2016

oliviertassinari Jan 31, 2016

rscnt commented Jan 31, 2016

oliviertassinari Jan 31, 2016

rscnt commented Jan 31, 2016

oliviertassinari commented Jan 31, 2016

rscnt commented Feb 1, 2016

oliviertassinari Feb 1, 2016

rscnt Feb 1, 2016

oliviertassinari commented Feb 1, 2016

mbrookes commented Feb 11, 2016

oliviertassinari Feb 12, 2016

rscnt Feb 12, 2016

oliviertassinari commented Feb 12, 2016

oliviertassinari Feb 12, 2016

rscnt Feb 12, 2016

rscnt commented Feb 14, 2016

oliviertassinari Feb 14, 2016

oliviertassinari commented Feb 14, 2016

alitaheri commented Feb 14, 2016

rscnt commented Feb 15, 2016

oliviertassinari commented Feb 15, 2016

[TimePicker] Update time state on new defaultTime prop. #3095

[TimePicker] Update time state on new defaultTime prop. #3095

Conversation

rscnt commented Jan 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrookes commented Jan 30, 2016

rscnt commented Jan 31, 2016

mbrookes commented Jan 31, 2016

oliviertassinari commented Jan 31, 2016

Choose a reason for hiding this comment

rscnt commented Jan 31, 2016

Choose a reason for hiding this comment

rscnt commented Jan 31, 2016

oliviertassinari commented Jan 31, 2016

rscnt commented Feb 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Feb 1, 2016

mbrookes commented Feb 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Feb 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rscnt commented Feb 14, 2016

Choose a reason for hiding this comment

oliviertassinari commented Feb 14, 2016

alitaheri commented Feb 14, 2016

rscnt commented Feb 15, 2016

oliviertassinari commented Feb 15, 2016