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 resource #423

Merged
merged 1 commit into from
Jan 3, 2018
Merged

Add resource #423

merged 1 commit into from
Jan 3, 2018

Conversation

TeaBough
Copy link
Contributor

@TeaBough TeaBough commented Jun 2, 2017

I've created this PR, that serve the same purpose as #373. The difference is that the logic of the resource is done directly in the TimeGrid instead of creating a ResourceGrid which DRY things up and allow to enjoy resource in the week view as well.

@TeaBough TeaBough mentioned this pull request Jun 2, 2017
3 tasks
@jquense
Copy link
Owner

jquense commented Jun 19, 2017

🎉 thanks for picking this up!

Copy link
Owner

@jquense jquense left a comment

Choose a reason for hiding this comment

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

Mind posting some screens as well to get an idea of it? We'll probably have to tweak the styles for when tehre are lots of resources to allow horizontal scrolling

src/TimeGrid.js Outdated
@@ -174,10 +176,14 @@ export default class TimeGrid extends Component {

let gutterRef = ref => this._gutters[1] = ref && findDOMNode(ref);

let eventsRendered = resources ?
Copy link
Owner

Choose a reason for hiding this comment

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

resources && resources.length ?

src/TimeGrid.js Outdated
return resources.map((resource, id) => {

let eventsToDisplay = daysEvents.filter(
event => event.resourceId === resource.id
Copy link
Owner

Choose a reason for hiding this comment

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

Like the events I don't want to be prescriptive about the shape of the data in resources so can we add accessor props for these fields like we do with startAccessor, et al?

src/TimeGrid.js Outdated

</div>
</div>
);
}
renderEventsWithResource(range, events, today, resources) {
Copy link
Owner

Choose a reason for hiding this comment

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

can this method be joined with renderEvents I'd be worried the common code will diverge unintentionally

@XVincentX
Copy link

@TeaBough Is this still happening?

@TeaBough
Copy link
Contributor Author

I couldn't work on it during the french summer. But I think I can work on this this weekend !

@XVincentX
Copy link

I think this feature would be a significant game changer here 👍

@TeaBough
Copy link
Contributor Author

@XVincentX @jquense I've finally updated this PR

@jquense
Copy link
Owner

jquense commented Sep 23, 2017

Awesome thanks! I'll try and review asap

@jquense
Copy link
Owner

jquense commented Oct 2, 2017

looks great! Can you rebase? The one concern i see here is that with a few resources the rendering hangs for a few seconds as you switch to the tab. I'm not sure what it is that is causing it, it may just be the number of divs being rendered as the grid since I see the same thing with or without any events.

@TeaBough TeaBough force-pushed the add_resource branch 2 times, most recently from 15c3b68 to 7396dee Compare October 2, 2017 16:52
@TeaBough
Copy link
Contributor Author

TeaBough commented Oct 2, 2017

I've rebased

@rvignesh89
Copy link

This is great! Exactly what I've been looking for. @jquense any chance that this will be added soon?

@arthurmmedeiros
Copy link

Hey guys,
When is this feature going to be added to the master?

@ksloan
Copy link

ksloan commented Nov 12, 2017

Any updates?

@samlll42-github
Copy link

Looking forward to this as well. Happy to help moving this along if it requires some testing or updates....

@jquense
Copy link
Owner

jquense commented Nov 15, 2017

If anyone wants to help, the last remaining thing is the performance concerns I noted in my comment. If anyone wants to take a crack at optimizing/testing that a bit

@vongdarakia
Copy link

vongdarakia commented Nov 17, 2017

There's a slight performance issue, but it's a not a big deal for me. It's still way faster than other calendars out there. They often do an HTTP request (page refresh). I'd really like to see this feature out though. It'd be very helpful! Thanks for all the hard work @jquense and @TeaBough!

Copy link
Contributor

@frehner frehner left a comment

Choose a reason for hiding this comment

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

React.PropTypes won't work in React16, and the rest of this file just uses PropTypes

src/DayColumn.js Outdated
@@ -64,6 +64,7 @@ class DayColumn extends React.Component {
dayWrapperComponent: elementType,
eventComponent: elementType,
eventWrapperComponent: elementType.isRequired,
resource: React.PropTypes.string
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to remove React. here: just PropTypes.string

@frehner
Copy link
Contributor

frehner commented Nov 28, 2017

Here's a screenshot looking at the Chrome Dev Tool's performance tab when the resource example is created in Storybook.

screen shot 2017-11-28 at 11 10 50 am

I'm no expert, but it appears that there isn't anything new per se that this PR introduces that reduces performance, it's just the fact that you're now rendering N number of DayColumns (one per resource). I may be wrong, but if performance is a worry here then we'll either have to figure out a different way to do resources, or go into Day/Time Column components and optimize there.

Thoughts?

@jquense
Copy link
Owner

jquense commented Nov 28, 2017

yeah it may just be the extra columns now. The issue is that it's gonna worse and worse the more resources you have. One solution may be to do something completely different layout wise, like full calendar: https://fullcalendar.io/scheduler/ something a bit more amenable to simpler layout.

another option is to maybe try and virtualize the TimeColumns so it's not rendering extra divs. We can also jsut try and reduce the amount of DOM elements rendered in the first place

@frehner
Copy link
Contributor

frehner commented Nov 28, 2017

I'm currently looking at a way of possibly just having one DayColumn component and just using styling to put the events into different columns... does that seem like a reasonable solution?

@jquense
Copy link
Owner

jquense commented Nov 28, 2017

I'm up for whatever works and isnt too hacky :P

@frehner
Copy link
Contributor

frehner commented Nov 28, 2017

On second thought, changing the CSS works well when it comes to displaying the events, but would require more work when you consider creating events. Hmm. I'm not really sure what the best way to go about doing this is then, other than what has already been done here.

Perhaps just looking at optimizing TimeColumns to make them not so heavy...

@XVincentX
Copy link

Will this find its way in the master somehow? @TeaBough

@TeaBough TeaBough force-pushed the add_resource branch 2 times, most recently from 5ce0c96 to 02181b1 Compare January 3, 2018 13:28
@TeaBough
Copy link
Contributor Author

TeaBough commented Jan 3, 2018

@XVincentX I've rebased with master and removed React. from React.PropTypes. I don't know what else I can do to make this PR ready to merge...

@jquense
Copy link
Owner

jquense commented Jan 3, 2018

if we can make CI green i'm ok merging for now. We really need to address the performance issues though

@TeaBough
Copy link
Contributor Author

TeaBough commented Jan 3, 2018

@jquense done

@jquense jquense merged commit 0c0b17c into jquense:master Jan 3, 2018
@marwej
Copy link

marwej commented Jan 24, 2018

Any thoughts on when next version (including this) will be released? (Many thanks for a great package!)

@f2net
Copy link

f2net commented Jul 27, 2018

Could you please add the new "resources" API in the documentation?
I'm looking at the source to understand it, but it would be great to have it documented.

Thank you,
Matteo

@atul-vzn
Copy link

Does anyone have a working example?

Thank you
Atul

@justoverclockl
Copy link

Could you please add the new "resources" API in the documentation? I'm looking at the source to understand it, but it would be great to have it documented.

Thank you, Matteo

any chance to know if this is implemented in 1.8.1? with a sort of documentation? thx!

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.