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

Added scrollTime prop like in FullCalendar #122

Merged
merged 3 commits into from
Jun 28, 2016

Conversation

manutenfruits
Copy link
Contributor

I added a new prop scrollTime so that the TimeGrid view is initially scrolled down to a specified hour on every mount or change of prev/next week. Let me know if I need to add any checks!

Original prop: http://fullcalendar.io/docs/agenda/scrollTime/

Takes a Date object, which I don't find really useful (a moment.duration would be more suitable, or an amount in millis, but I get how this is moment-agnostic) but props min and max used that too so I figured I'd use the same method :)

@@ -30,3 +30,7 @@ node_modules

# Webstorm ide files
.idea

# Example bundles
Copy link
Owner

Choose a reason for hiding this comment

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

these are actually meant to be committed, they are for the docs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops make sense, I'll add them then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On a second note, I think you can generate them in npm preinstall but we can leave that for later.

@manutenfruits
Copy link
Contributor Author

@jquense I fixed all the issues you pointed out, let me know if there's anything else that needs work done

@@ -14,6 +14,7 @@ let Selectable = React.createClass({
selectable
events={events}
defaultView='week'
defaultTimeScroll={new Date(1970, 1, 1, 17)}
Copy link
Owner

Choose a reason for hiding this comment

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

this isn't a better name I don't think. the "default" version of props also have a non default counterpart (this does not). Default props also imply that the prop is only set once which isn't true in this case. can we use scrollToTime instead?

@manutenfruits
Copy link
Contributor Author

Fixed. I also squashed it all under one commit.

componentWillReceiveProps(nextProps) {
const { start, scrollTop } = this.props;
// When paginating, reset scroll
if (!dates.eq(nextProps.start, start) || nextProps.scrollTop !== scrollTop) {
Copy link
Owner

Choose a reason for hiding this comment

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

dates.eq(nextProps.start, start, 'day') and probably the same for: nextProps.scrollTop !== scrollTop no? Well not 'day' but perhaps 'minutes', ie only recalculate with the scrollToTime has changed more than a minute.

Also scrollTop is wrong here right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

scrollTop wrong, yes. The condition, however is correct, because it only calculates the new scroll value when the start is different (i.e. it has changed dates or paginated) or when the scrollToTime prop has changed.

@jquense
Copy link
Owner

jquense commented Jun 28, 2016

ok thanks for the updates, I finally had a chance to give it a real close look, just a few concerns

@jquense jquense merged commit 82ecd3e into jquense:master Jun 28, 2016
@jquense
Copy link
Owner

jquense commented Jun 28, 2016

many thanks

@manutenfruits
Copy link
Contributor Author

Thank you for creating this plugin! Are you planning a release anytime soon?

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.

2 participants