Multiday events

[asavin]

Motivation

Currently it is only possible to specify datetime of the event. With this change we are introducing ability to specify start datetime and end datetime. Events spanning multiple days will have those days displayed in the UI (under Today list).

• Event stays in “Upcoming” list, using the first day of event for order.
• When the first day of event is today, move to “Today” list. Still showing the same date range
• When the last day of event is gone, move to “past” list. Use the last day of event for order.
• Tests
• Gif added

[lpil]

I think it would be better to fix the error than to disable the rule.

Also this component looks like a better fit for a function rather than a class.

[asavin]

I don't think you can fix line length with multiline strings, can you?

[lpil]

You can wrap a string over multiple lines with either + or \.

Also this is an expression which is easier to wrap. I think it should be built by a function rather than inline like this. Really hard to read between the template strings, use of this.props, and the ternary operator.

[lpil]

if (displayDateRange) {
  return `${startDate} ${startMonth} ${startYear} - ${endDate} ${endMonth} ${endYear}`
} else {
  return `${startDate} ${startMonth} ${startYear}`;
}

I think this is much clearer and breaks no linting rules

[asavin]

👍

[jameshopkins]

You could go one further and use this.props.ISODate or whatever the field is called, along with format and use the in-built tokens

[lpil]

These look required to me?

[AndrewBestbier]

Perhaps we could use our pathOr(['...]), that we have used in badger brain to access these nested object keys. Just thinking of a scenario where someone changes the query and doesn't actually query for the startDateTime but yet we try access the .iso key

[asavin]

I think that's a bit of premature optimization - this is a client and it knows what it just requested

[asavin]

oh you mean in case startDateTime is null? Maybe.

[AndrewBestbier]

Yeah. Basically whenever I see a.b.c.d.e these days I want to use pathOr

[lpil]

Totally should be built in :)

[asavin]

I'm putting this one on hold for now - at the moment if startDateTime is missing, the whole site will explode. We should handle this in a clever way, probably on BB level.

[asavin]

This should be addressed as part of a separate PR.

[AndrewBestbier]

Might be cleaner if we put this in its' own function

[lpil]

Would it make more sense to do this inside the child component?

[asavin]

Yeah probably. There's also potential for refactoring in a way of only passing down elements of the event and not the whole event.

[asavin]

Note that we need to update to React 15 in order to get that null returns working without warnings.

[lpil]

Obey the style guide instead of turning off the linter.

[asavin]

What are my options?

[asavin]

nvm, fixed

[AndrewBestbier]

Looks good :)

[asavin]

@AndrewBestbier merge merge!