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

Resizable Events #406

Closed
wants to merge 22 commits into from
Closed

Resizable Events #406

wants to merge 22 commits into from

Conversation

jnmandal
Copy link

@jnmandal jnmandal commented May 21, 2017

This PR adds the ability to resize events to the DND plugin, as discussed in issue #382. It includes:

bug fix to core:

  • eventComponent was being destructured incorrectly in the DayColumn component's renderEvents function (so it was always undefined)

enhancements to dragAndDrop addon:

  • passing prop resizable will have the wrapper use a new component ResizableEvent as the eventComponent for the calendar
  • the onEventResize callback will give the ability to hook into the DND wrapping. It receives:
    • resizeType: 'drag' or 'drop' based on what type of interaction is causing the resize
    • eventData: object containing the original event as well as a new end for the event

gotchas:

Since only the original event object is accessible through react-dnd... In the onEventResize callback you will not be able to use Array.prototype.indexOf for finding the event again -- unless you continue to mutate the original event and splice it into the cloned events array... but I'm not sure if we would want to recommend that practice

@jquense
Copy link
Owner

jquense commented May 25, 2017

hey there thanks a lot for the PR! Sorry i'm a bit slow to review I'm a bit sort on time at the moment but it's really appreciated.

I do think I would at least split this into a separate addon, so folks can use resizing without drag and drop if they want.

@jnmandal
Copy link
Author

Hey @jquense no worries. Thanks for building out this calendar component.

I did think about the concept of DnD and Resizable being separate but the way I implemented it, one would be useless without the other. My goal here was to offer the functionality found in google calendar; there, you can only resize from the bottom so to adjust an event's duration and start time you need both resizeable and drag and drop functions.

We could definitely break it out and offer a Calendar.app style experience where events are resizable at top and bottom but I'm worried it will be a bit of duplicated code/functionality between the DnD addon and the Resizable addon.

@jquense
Copy link
Owner

jquense commented Jun 19, 2017

@jnmandal sorry for the delay 😳 Why would they be useless seperated? It seems like you make use of react-dnd but that's ok, if a user uses both addons the dependencies will be shared.

I don't mind them being together honestly but I'd like to be able to disable drag and drop and only use resize if I wanted.

@jnmandal
Copy link
Author

Hey @jquense no worries. I just mean the functionality this PR provides is just to allow the end date of an event to be changed. I did not add a resizable anchor to the top.

On Calendar.app you can resize both top and bottom (start and end) but in google calendar, only bottom (end). I only implemented the "google calendar" resizing so its not very useful decoupled from the ability to drag and drop the whole event as well.

@jquense
Copy link
Owner

jquense commented Jun 28, 2017

ah I gotcha, that makes sense. still folks may just want to change the duration if not the start time of the event. Would it be a lot of trouble to make them individually togglable? We can totally keep them in the same addon so you should still get the benefit of them being coupled right?

@Raman16
Copy link

Raman16 commented Aug 8, 2017

Thank you so much for adding Resizable Event feature to BigCalendar.

@jnmandal
Copy link
Author

jnmandal commented Aug 8, 2017

Sorry for delay on fixing this up, I have been slammed; I will look into decoupling dragging from the resizing this weekend

@arecvlohe
Copy link
Collaborator

What is the estimated timetable for this feature? Just wondering

@jquense
Copy link
Owner

jquense commented Aug 22, 2017

whenever folks get it done :)

@jnmandal
Copy link
Author

Was hoping to resolve issues last weekend but I'm a little behind on some projects right now 😅 I think I can take a stab at this sometime this week

@arecvlohe
Copy link
Collaborator

I will take a look at your fork @jnmandal, let me know if I can help in anyway.

@jquense jquense mentioned this pull request Sep 11, 2017
@arecvlohe
Copy link
Collaborator

@jnmandal I think if it's okay, we will run with the fork that you have so this feature can be integrated in in our project. Thanks for working on this. We will try to contribute if we are able to take it further.

@jnmandal
Copy link
Author

Yeah @arecvlohe I am unfortunately still in process of converting this into a separate addon -- took a stab yesterday while on an airplane. I'm grappling w/ how to do it w/o duplicating so much of the dnd addon and then also making them play nice together. TBH, I am also planning on running with fork for now as I'm still slammed and need to ship.

If someone else has idea for implementation or wants to take over please go for it.

@gazab
Copy link

gazab commented Sep 12, 2017

I think it's foolish to try to seperate them completely. Isn't it better to just make it possible to turn them off separately while still being part of the dnd addon? @jquense said that was fine.

@jquense
Copy link
Owner

jquense commented Sep 12, 2017

Yes! sorry if was unclear i'm ok with this being in the same addon as DND

@alxmiron
Copy link
Contributor

alxmiron commented Oct 2, 2017

I'm ready to help with this. @jnmandal, I will look into your fork

@arecvlohe
Copy link
Collaborator

So it looks like having this in the dnd addon is fine. So that's clear. What @jquense is asking for is that both ends be toggle-able in day view.

I will be working on adding this toggle-ability to the month view where users can adjust the dates by toggling the left and right parts of the event. If I can get this working relatively soon then I will tackle the adjustment to make both end toggle-able in the day view.

Example:
event-lengthen

Summary: There were errors in my branch because importing the .default was not pulling in the correct default whereas when I removed it, it seemed to work fine. This might be some kind of change in React 15.6 for how default modules are imported/required.
Summary: For <EventCell /> to know that it can be resized I am passing down the resize prop to that component. This is because, unlike in the previous implementation, I am not passing down what would be the eventWrapper from the dragAndDrop addon. I didn't know how to pass down multiple components based on whether the calendar was resizable. So I went this route. Sorry, not sorry.
Summary: I created <ResizableEventLR /> to handle resizing events in the month view. If <EventCell resizable /> then it will render the resizable component.

I added styles so that I could see what is going on. There is a red bg. Sorry, not sorry.

There is no functionality at the moment but I started building it out. There is certainly more to be worked on here.
Summary: Updated the resize function to be aware of the end as well as the start time. This is when there are two dnd handlers on an event at once.

I renamed the component to be more semantic with its usage. Styles were correspondingly updated.
Summary: I didn't realize the conent should be aligned left.
@arecvlohe
Copy link
Collaborator

I have a working prototype for resizing month events. @jnmandal These changes were made off of your fork, so I could push to your repo to get these changes into your fork/branch. I will also work on adding toggling to the top and of the event in week/day view. Please add any comments you might have @jquense. Also I am wondering if I should just push my branch so you can see what the code looks like. Just let me know.

resize-month-event

Summary: Refactored components to look a bit more readable.
Summary: Refactored <ResizableEvent /> and added the ability to resize the top component.
Summary: Refactored the if/else statement into a switch statement.
@arecvlohe
Copy link
Collaborator

I have completed a prototype for the month and week/day resize events that can be resized by dragging the left/right and top/bottom of the elements respectively. Here is what they look like thus far:

Week

resize-event-week

Month

resize-month-event

I am hesitant to start another PR but if I don't connect with @jnmandal then I think I might have to. What are your thoughts @jquense?

@arecvlohe
Copy link
Collaborator

arecvlohe commented Oct 4, 2017

If you want to check out the relevant changes go here

Summary: I deleted these chanages thinking they were old. My mistake!
@jnmandal
Copy link
Author

jnmandal commented Oct 5, 2017

@arecvlohe thanks for doing this. Looks like a big improvement. I added you as collaborator to my fork so you can push there and update this PR and/or feel free to open additional. Sorry I have been MIA.

Summary: Remove unused code.
@arecvlohe
Copy link
Collaborator

@jnmandal Thanks! I pushed to your branch for now. I am going to look into #577 and see how I can improve the hover functionality.

Resize Events in Month View and Resize Week/Day Events on Both Sides
@jnmandal
Copy link
Author

@jquense check out #578 its the same as this PR but cleaner w/ a rebase @arecvlohe kindly picked up my slack and made the improvements requested. thanks all

@jnmandal jnmandal closed this Oct 17, 2017
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

6 participants