Date time picker #895
Date time picker #895
Conversation
thecalcc
requested review from
tomaskikutis and
akintolga
and removed request for
akintolga
| @@ -160,7 +161,8 @@ export class DatePicker extends React.PureComponent<IDatePicker, IState> { | |||
| tabindex={this.props.tabindex} | |||
| > | |||
| <Calendar | |||
| footerTemplate={this.props.required !== true ? () => ( | |||
| className='sd-input__input' | |||
| footerTemplate={this.props.hideClear !== true ? () => ( | |||
There was a problem hiding this comment.
maintain logic regarding "required"
There was a problem hiding this comment.
This was a choice I made, because I didn't want to bloat the logic. I chose to bloat the interface instead and require users of the component to have to also pass hideClear. There's also a benefit to this I think - I don't couple the required logic with hiding the clear when you don't want it for other reasons.
There was a problem hiding this comment.
It's a violation of a code contract between interface(props) and implementation. As a developer I say via props that field must not be empty, and you allow it to be empty - ignoring what was requested via props.
Bloat is a secondary issue compared to this. I don't see it as bloat either - being able to clear the value is a core function of any input.
|
|
||
| export class DateTimePicker extends React.PureComponent<IProps> { | ||
| handleTimeChange = (time: string) => { | ||
| const [hours, minutes] = time.split(':').map((x) => defaultTo(parseInt(x, 10), 0)); // handle NaN value |
There was a problem hiding this comment.
FYI there's also isNaN function
There was a problem hiding this comment.
I'm aware. But in the case of using isNaN the whole expression becomes less readable in my opinion, because isNaN only checks whether the value is NaN, defaultTo does the same but also offers an immediate fallback. So in this case it is a better fit. Otherwise if you prefer a more explicit style, with isNaN it would look like isNan(x) ? 0 : parseInt(x, 10).
There was a problem hiding this comment.
You can leave it as you prefer, I don't mind. I just wanted to point that isNan function exists. Personally I slightly prefer isNan approach, because it doesn't use libraries and person reading the code doesn't need to understand what the library function does. I dislike lodash specifically for making the same function compatible with many data types. It's good on one side, but on another, it's making it harder to look at code and infer from usage what's going on.
| import * as React from 'react'; | ||
| import {DatePicker} from '../components/DatePicker'; | ||
| import {Spacer} from '@superdesk/common'; | ||
| import {defaultTo, padStart} from 'lodash'; |
There was a problem hiding this comment.
can you use padStart from standard library?
| @@ -33,6 +33,8 @@ interface IDatePickerBase extends IInputWrapper { | |||
| }} | |||
| */ | |||
| locale?: DatePickerLocaleSettings; | |||
|
|
|||
| // Hide does not mean show | |||
There was a problem hiding this comment.
light does not mean dark ;D You'd need to phrase clearer. Or drop it completelly which would be fine here too.
STT-109