Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add availableHours and availableDays #6

Merged
merged 19 commits into from Mar 15, 2018
Merged

Add availableHours and availableDays #6

merged 19 commits into from Mar 15, 2018

Conversation

causztic
Copy link
Contributor

Added availableHourRange and availableDays as extra props.
availableDays will restrict which days are available to choose time slots, graying out the days not in the array.
availableHourRange will restrict by graying out the hours outside the range.
Updated README, version number and test page accordingly.

@trotzig I'm not very familiar with front-end testing, how would you go about testing this new feature?

Fixes #4

Copy link
Owner

@trotzig trotzig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good! Thanks for the thorough effort. 👍

I left a few inline comments/questions with suggestions for improvements.

As for testing, I've been taking a mostly manual approach. The utility functions (e.g. weekAt, makeRecurring, etc) are all easy to unit-test. The overall functionality (including styling, behavior) is done manually with the help of the start-test script (e.g. yarn start-test) which makes it possible to play around with the component on a test page.

src/Day.jsx Outdated
className={styles.mouseTarget}
ref={this.handleMouseTargetRef}
/>
<this.EventTracking available={available} hourLimits={hourLimits} />
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of initializing the component on the instance, did you consider something like

{available && (
<div
  onMouseDown={this.handleMouseDown}
  onMouseUp={this.handleMouseUp}
  onMouseMove={this.handleMouseMove}
  onMouseOut={this.handleMouseUp}
  onTouchStart={this.handleTouchStart}
  onTouchMove={this.handleTouchMove}
  onTouchEnd={this.handleTouchEnd}
  className={styles.mouseTarget}
  ref={this.handleMouseTargetRef}
  style={{ 
    top: hourLimits.top,
    height: hourLimits.difference,
  }}
/>
)

While I appreciate the effort to reduce the size of the render method, I think having things like these inline is more straightforward.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I'll make the necessary changes.

src/Week.jsx Outdated
bottomHeight: (24 - availableHourRange.end) * HOUR_IN_PIXELS, // bottom height
difference: (availableHourRange.end - availableHourRange.start) * HOUR_IN_PIXELS,
};
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In an effort to keep the props for <Day> cleaner, would it make sense to do this inside Day.jsx? Don't worry about the additional computation needed, I think it's neglible in the grand scheme of things.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did calculate it in <Day> initially but thought there was quite a bit of duplicated code (e.g. bottom distance was used a few times and I didn't like the idea of writing a function just for a simple calculation) so I thought to pass it down as props. Furthermore, it's the same constraints for all <Day> objects anyway.

I can still move it back to <Day> if we want to prioritise a cleaner props however.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, this is fine. Thanks for the explanation.

@trotzig
Copy link
Owner

trotzig commented Mar 14, 2018

Sorry about the package.json conflict, I had forgot to push that in a previous update. It's just the version number conflicting, so feel free to overwrite. In general, you can usually ignore bumping the version number and leave that to the publisher/maintainer.

@trotzig
Copy link
Owner

trotzig commented Mar 14, 2018

I gave this branch a spin locally and it seems to work well. The only issue I found is that I can't extend an event to reach the edge of the available hours during a day.

screen shot 2018-03-14 at 13 45 58

See the green area -- I can't pull it down further than that.

@causztic
Copy link
Contributor Author

Funny thing was, last time I contributed to another project the maintainer asked me to bump the version number myself... now I don't know who to believe!

Yeah I noticed the gap in the event making not it possible to reach the edge, I will take a look when I have the time tomorrow.

@trotzig
Copy link
Owner

trotzig commented Mar 14, 2018

Funny thing was, last time I contributed to another project the maintainer asked me to bump the version number myself... now I don't know who to believe!

Haha 😄

My reasoning is that it's hard for an outside contributor to keep track of other changes to know enough about whether to bump the major, minor, or patch number. Also, the maintainer might want to publish a pre-release or something.

I'm going to go out on a limb here and say the other maintainer was being lazy 😬

@trotzig
Copy link
Owner

trotzig commented Mar 14, 2018

A note on styling -- what would you think about changing the gray background to a striped background? Something like

  background-image: linear-gradient(-45deg, #f6f6f6 25%, transparent 25%, transparent 50%, #f6f6f6 50%, #f6f6f6 75%, transparent 75%, transparent);
  background-size: 10px 10px;

@trotzig
Copy link
Owner

trotzig commented Mar 14, 2018

Just out of curiosity - how are you planning on using this new feature? What's the application behind this? :)

@causztic
Copy link
Contributor Author

Just out of curiosity - how are you planning on using this new feature? What's the application behind this? :)

Oh my current school's project's to create a scheduling application for the faculty to choose their preferred time slots for lessons, after which the course coordinators can then refer to them as a soft constraint to plan out lessons.

It required us to block out specific days (namely weekends) and confine it within the school's working hours, so I thought I could make a pull request for this feature 😄

@causztic
Copy link
Contributor Author

A note on styling -- what would you think about changing the gray background to a striped background?

Ohh that looks nice, I'll change to that.

@causztic
Copy link
Contributor Author

I have added a fix for the stretching by slightly expanding the draggable region, do take a look!

@trotzig trotzig merged commit 0da1247 into trotzig:master Mar 15, 2018
@trotzig
Copy link
Owner

trotzig commented Mar 15, 2018

Thanks for the updates! I'll publish a new version in a minute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants