unify DaySlot/TimeGutter #107

cellog · 2016-06-05T04:55:05Z

Success!

This pull request unifies DaySlot and TimeGutter, using new TimeSliceGroup and TimeSlice. It exposes new configurable properties on Calendar, "slices" and "now" for controlling the number of time slices, and what the calendar displays as today/right now. It defaults to 2 slices and the current date/time for backwards compatibility.

jquense · 2016-06-05T14:04:30Z

woohoo! that looks great, minimal changes but definitely better split up internally.

I just have a few comments and I think it's good to go

jquense · 2016-06-05T14:05:59Z

src/TimeGutter.jsx


-let TimeGutter = React.createClass({
+export default class TimeGutter extends Component {


I think now that this is used for all the large slots, TimeGutter is no longer a good name for this component. Perhaps TimeSegment, or TimeColumn (to go with TimeGrid?) makes more sense.

TimeColumn: check

I also think we should rename the DaySlot and TimeSlice and TimeSliceGroup to DayColumn, TimeSlot, TimeSlotGroup

jquense · 2016-06-05T14:28:36Z

I should have it as a configured npm script but could you also run eslint on src it should revealed a left over few things to clean up as well

jquense · 2016-06-05T14:32:42Z

src/less/day-slot.less

+.rbc-timeslice-group {
+  border-bottom: 1px solid @cell-border;
+  min-height: 40px;
+  height: 40px;


the height may not be necessary here... at least not with min height. actually does this make more sense on TimeSlice vs the group?

this is taking the css from rbc-time-slice, which was min-height/height: 20px (see comment below). If we do height on time-slice, then multiple slices breaks. We flex the height now with the new commit

What do you envision the behavior of the slice to be? seems like there are two options,

what is there now which is min heights on the Group which shrinks slices proportionally

height on the slice which makes them scale bit better, but groups expand in height without bound.

I am not sure which makes the most sense honestly. It does seem like setting 20px min height on the slices and let the group have whatever height would be a good default as well. it sort of depends on how you envision the slices.

The entire point of the slices was to shrink them, so that you can fit an entire day onto a single page without scrolling, to allow selecting a large time chunk. Otherwise you have to change the selection code to scroll the page when the mouse gets to the bottom. To do the behavior you're describing, all we have to do is change the step value. The slice (or time slot, as I'm about to rename it) value is about making it tinier. This is one of the simplybook.me-inspired features.

ahh ok. So there is a bit of a conflict inherent in that, since some also would rather that the days not shrink below a certain size and scroll beyond that. We should make sure then we have css defaults that are easy to adjust/override for folks that want either behavior.

In your case getting everything to fit on a page should be a simple matter of setting the calendar height to the page size and the flexbox styles could handle everything from there for all the inner components. as it is that won't work without some adjustments up the line... in any case I can tweak that stuff later, nothing to be done here now.

This is what I meant: https://github.com/intljusticemission/react-big-calendar/tree/full-height

I just added 3 new storybook stories to show how I envisioned the thing working. The purpose is to take the same space that has 2 30 minute selectable time slots, and render it with different lengths. So, for instance, you change the step to 10, and use 6 time slots, or you change the step to 20 and have 3 time slots. The visual representation of an hour is the same for each.

(fyi i'm not suggesting this PR is not needed, just that I misunderstood it a bit :P you've opened up a bunch of fun new style options)

I think we are on the same page, I would just say that at a minimum the columns should extend the heightof the calendar so we don't have have a mostly empty bordered box, Whether or not the columns "grow" the calendar itself or overflow with a scrollbar is up to the user and whether they apply min-heights on the slices/groups. From a default perspective tho I think the tweaks in my branch (minus the removal of the min-height) should be reasonable

Gotcha. So we use simple flex for the time slice groups and let the Calendar box size them, with a min-height of 40 and no max height

…actor spots

cellog · 2016-06-05T14:49:22Z

src/less/day-slot.less

 }

 .rbc-time-slot {
  flex: 1 0 0;
-  min-height: 20px;
-  height: 20px;


this was where the 40px 40px comes from. rbc-time-slot was 20px, and we want the height of the group to equal 2 of these. I agree min-height and height is a weird redundancy, but it came from here. No problem correcting it if you want, just moving to height

…es, we need some new default values. In particular the now prop is needed to display the correct current date/time

cellog · 2016-06-05T15:09:24Z

try it now, all the issues you mentioned are fixed. I am going to rename the cosmetic DaySlot and so on.

One question: css is still based on old component names, can we make aliases to the new component type and have the old ones just for backwards compatibility? so rbc-time-slot instead of rbc-time-slice, rbc-time-column instead of rbc-time-gutter, etc.

cellog · 2016-06-05T15:09:54Z

oh and both docs and examples need to be updated once all the code is approved before you merge the commit, I can do that easily.

cellog · 2016-06-05T15:20:22Z

I think there is a bug in the eslintrc. The semi rule is turned off, but set to never, I think it should be either 1 or 2 for warning or error?

"semi": [0, "never"],

I noticed semi-colons are inconsistent, probably because of this. Whatever you want though

cellog · 2016-06-05T15:24:13Z

didn't find any other eslint issues

jquense · 2016-06-05T15:45:31Z

src/TimeColumn.jsx

+
+    switch (this.props.type) {
+      case 'gutter' :
+        baseCss = 'rbc-time-gutter'


any reason why we can't let the parent DaySlot add the rbc-day-slot class? I don't think the conflict with each other so having both might not matter in that case. It's a bit less magical then the type.

I appreciate that you are trying to maintain the old markup, tho maybe the old markup was a bit off?

that may be the answer, yes

Nope, tried this, broke everything. rbc-time-gutter and rbc-day-slot conflict :)

ok then thats fine for now, I'll take a stab at cleaning up the styles after

jquense · 2016-06-05T15:47:20Z

src/TimeSlice.jsx

+
+  render() {
+    return (
+      <div className={cn('rbc-time-slot', this.props.isNow ? 'rbc-now' : '')} {...this.props.style}>


cn('rbc-time-slot', this.props.isNow && 'rbc-now') works as well here

jquense · 2016-06-06T12:22:13Z

src/Day.jsx

+      now: new Date(),
+      type: 'gutter',
+      timeslots: 2
+    }


To me its a big confusing to define defaults for props that the component doesn't explicitly use. Its makes it a bit harder to understand which value will be used ultimately and harder to change the default in the right spot.

Can we remove them for these components?

This is the problem with re-using attributes from other components. These properties are absolutely required for TimeGrid to function. Thus, when clicking the buttons week/day in the toolbar, there are a slew of warnings that these proptypes are not present, and in fact the calendar fails to display the current time unless this is present.

2 choices (yours to make):

also use TimeGrid's default props
getDefaultProps() {
return TimeGrid.defaultProps
}

declare these properties on the Calendar component with the proper defaults

Ahh right. I believe in that case it makes sense to not mark them as required (since they are actually provided), and provide the defaults at the lowest level they are needed. That shouldn't trigger warning right?

second choice for me would be 1, since it mirrors how we spread propsTypes

I would like to go with the 2nd choice. The reason is that even though you don't wish to document the internals so we don't run into backwards compatibility hell, if anyone does need to make a change, it's much harder to do it unless all the needed props are documented and required ones explicitly marked as so. This is why I always mark props that are absolutely needed for the component to function as required, even if they have a default value supplied. It's redundant, but this stuff is only checked in development anyways, so I think of it as another layer of documentation for the developer. So is it cool to use the default props for that reason?

fine with me 👍

jquense · 2016-06-06T12:35:05Z

Noted a last few nits, can you also squash the PR down and then we should be good to go

cellog · 2016-06-06T13:01:34Z

I think we're in syzygy now...?

jquense · 2016-06-06T13:02:14Z

src/Week.jsx

-      type: 'gutter',
-      timeslots: 2
-    }
+    return TimeGrid.defaultProps


in case you didn't know, you could have done TimeGrid.getDefaultProps() I believe. switching to a class is fine too

jquense · 2016-06-06T13:14:44Z

just the one last note about the event handlers that break when switching to es classes

cellog · 2016-06-06T15:34:07Z

ok, ready

jquense · 2016-06-06T15:37:28Z

oops it looks like you missed the _headerClick handler as well.

jquense · 2016-06-06T15:37:38Z

(accidental close)

jquense · 2016-06-06T16:12:49Z

👏 thank you!

unify DaySlot/TimeGutter

26c5727

cellog mentioned this pull request Jun 5, 2016

Implement unified TimeGutter in Week and Day view using refactored code #102

Closed

jquense reviewed Jun 5, 2016
View reviewed changes

cellog added 2 commits June 5, 2016 09:23

rename TimeGutter TimeColumn

b028465

unused prop

9718a53

jquense reviewed Jun 5, 2016
View reviewed changes

cellog added 4 commits June 5, 2016 09:38

refactor hide labels to showLabels, missed a couple of TimeGutter ref…

29c27f9

…actor spots

correct default value

a705550

remove default for culture

e7689d1

fix padding issue in rbc-time-slice

60ad8a6

cellog reviewed Jun 5, 2016
View reviewed changes

cellog added 2 commits June 5, 2016 10:06

eliminate extra div for day-slot

948ff15

because these components use spread on TimeGrid or TimeColumn proptyp…

fffbe9d

…es, we need some new default values. In particular the now prop is needed to display the correct current date/time

cellog added 2 commits June 5, 2016 10:26

refactor DaySlot to DayColumn (cosmetic)

0312ade

more missing default props

e7d2003

jquense reviewed Jun 5, 2016
View reviewed changes

timeslice -> timeslot (cosmetic)

f04b095

jquense reviewed Jun 5, 2016
View reviewed changes

cellog added 4 commits June 5, 2016 10:51

cosmetic tweak

8162b14

add demo of intended usage of step and timeslots together

95ec1e7

one more demo

7fbd10b

stretch the time grid to fill available space

a4d71c2

jquense reviewed Jun 6, 2016
View reviewed changes

cellog added 3 commits June 6, 2016 07:49

showLabels default false

661d675

use TimeGrid defaultProps (had to move TimeGrid to es6 for this to work)

d5b0dc9

explanatory note

fb09946

jquense reviewed Jun 6, 2016
View reviewed changes

bind event selection method

5df5a71

jquense closed this Jun 6, 2016

jquense reopened this Jun 6, 2016

bind headerClick

12a5c8e

jquense merged commit 7fbe7f8 into jquense:master Jun 6, 2016

jdeniau mentioned this pull request Jun 9, 2016

ERROR in ./~/react-big-calendar/lib/TimeColumn.js #113

Closed


		let TimeGutter = React.createClass({
		export default class TimeGutter extends Component {

unify DaySlot/TimeGutter #107

unify DaySlot/TimeGutter #107

Conversation

cellog commented Jun 5, 2016

jquense commented Jun 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jquense commented Jun 5, 2016

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

cellog Jun 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cellog commented Jun 5, 2016

cellog commented Jun 5, 2016

cellog commented Jun 5, 2016

cellog commented Jun 5, 2016

jquense Jun 5, 2016 • 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

Choose a reason for hiding this comment

jquense commented Jun 6, 2016

cellog commented Jun 6, 2016

Choose a reason for hiding this comment

jquense commented Jun 6, 2016

cellog commented Jun 6, 2016

jquense commented Jun 6, 2016

jquense commented Jun 6, 2016

jquense commented Jun 6, 2016

cellog Jun 5, 2016 •

edited

Loading

jquense Jun 5, 2016 •

edited

Loading