allow creation of events without start/end time #2154
Conversation
| @@ -10,22 +9,41 @@ import {Button} from '../../'; | |||
|
|
|||
| import './style.scss'; | |||
|
|
|||
| interface IProps { | |||
| value: any; | |||
| onChange(value: string): void; | |||
There was a problem hiding this comment.
Probably value type can be improved here with string | null since on line 101 you're passing null
| @@ -64,7 +82,7 @@ export class TimeInputPopup extends React.Component { | |||
| const {onChange, close} = this.props; | |||
|
|
|||
| if (addMinutes) { | |||
| let newTime = moment.clone(this.state.currentTime); | |||
| let newTime = moment(this.state.currentTime); | |||
| 'dates.no_end_time': false, | ||
| }; | ||
|
|
||
| this.props.onChange(changes, null); |
There was a problem hiding this comment.
A second argument null is passed to onChange always here, so if it's easy could you remove that?
| localDateToUtc(this.props.item.dates.end, this.props.item.dates.tz) | ||
| : null, | ||
| 'dates.all_day': !hasStartTime, | ||
| 'dates.no_end_time': hasStartTime, |
There was a problem hiding this comment.
So with this logic if there's a start time there has to also be end time and vise versa? No option for one without the other?
There was a problem hiding this comment.
there can be no time - all_day is true, or no end time - no_end_time is true, or there must be both.
we can't have just end time
| * Combines date from the first argument with time from the second argument | ||
| */ | ||
| function combineDateTime(date: moment.MomentInput, time: moment.Moment, tz?: string): moment.Moment { | ||
| return moment.tz(moment(date).format('YYYY-MM-DD'), tz) // we only want the date part |
There was a problem hiding this comment.
Format is always YYYY-MM-DD on purpose, and we format based on format config later or it's a mistake here?
There was a problem hiding this comment.
here it's on purpose to only take the date from timestamp and ignore the time
| moment.tz(date, tz).format('YYYY-MM-DD') : | ||
| moment(date).format('YYYY-MM-DD') |
| null; | ||
| const error = get(this.props.errors ?? {}, field); | ||
| const momentValue = value != null ? moment(value) : undefined; // use moment.utc? | ||
| const error = get(this.props.errors ?? {}, field) || undefined; |
There was a problem hiding this comment.
Redundancy here - if get has to be used might be better to do
get(this.props.errors, field, undefined) if value isn't found, fall back value is returned, in this case would be undefined
|
|
||
| const expectedEvent = { | ||
| ...event, | ||
| 'dates.end.date': event['dates.start.date'], |
There was a problem hiding this comment.
should there be validation if an event starts & ends at the same time or we automatically treat it as a single day event? Maybe if there's time and start & end have the same values that's invalid?
There was a problem hiding this comment.
in general that's ok, there can be same values. we don't need to avoid it. but if there is a usecase that's another question 😄
petrjasek
merged commit d44272b
into
superdesk:feature/multiple-events-in-planning
STT-40
Front-end checklist
memoorPureComponentto define new React components (and updates existing usages in modified code segments)lodash.getwith optional chaining and nullish coalescing for modified code segmentssuperdeskApi)superdesk-ui-frameworkandsuperdeskApiwhen possible instead of using ones defined in this repository.planningApiwhere it is possible to usesuperdeskApiplanningApi)