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

Custom day component support #15

Closed
wants to merge 6 commits into from
Closed

Conversation

marek-sed
Copy link

@marek-sed marek-sed commented Jun 11, 2016

implementation of custom day component.

User can specify custom day component, by setting DayComponend property.
This component will be rendered inside of li element in calendar.
DayComponent is just for displaying data, mouseClicks are still provided by beforeSelect, onSelect, afterSelect events, specified on li component.

Styles for today, selected, disabled, enabled are responsibility of DayComponent.

All Props are being passed all the way down to DayComponent, so you user can pass specific data to DayComponent.

when we set min, minDate and locale to have dow=1
some months will overlap. This was caused by invalid number of
rows calculation
splitted day component into:
HOC withEvents: responsible to events to <li> and has the root style
DefaultDay: responsible for styles and state[selection, disabled, today]
@codecov-io
Copy link

codecov-io commented Jun 11, 2016

Current coverage is 93.06%

Merging #15 into master will increase coverage by 0.03%

@@             master        #15   diff @@
==========================================
  Files            19         19          
  Lines           818        822     +4   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            761        765     +4   
  Misses           57         57          
  Partials          0          0          

Powered by Codecov. Last updated by 576815b...9482252

@clauderic
Copy link
Owner

Hey @marek-sed, sorry I didn't get the chance to take a look at this sooner. Just curious, why did you choose the HOC approach?

@marek-sed
Copy link
Author

Hi, no problem I am enjoying the last day of my vacation right now :-)

I like the readability of it a lot more then wrapper component, I can immediately see that it has nothing to do with how component is displayed.

But in this case, I don't have any strong preference for it, neither any better argument that I like it.
If you prefer normal component I can rewrite it tomorrow.

@marek-sed
Copy link
Author

Hi @clauderic, how is it going.
Do you have any questions, anything I can help with. :)

@clauderic
Copy link
Owner

clauderic commented Jun 29, 2016

Hey @marek-sed, I took some time to review your PR and have given it some thought. I'm still on the fence as to whether or not I this is the right approach. I feel like it makes for a very complex setup for the end-user. I also think the HOC is not necessary. I'd be more inclined to only pass down a subset of props, something along the lines of:

const Day = (date, events = [], isSelected) => {
    return (
        <div>(...)</div>
    );
}

<InfiniteCalendar dayRenderer={Day} />

(events would be added once react-infinite-calendar gets support for events out of the box)

@marek-sed
Copy link
Author

marek-sed commented Jun 29, 2016

Hi, thx for review.
Do you agree on how dayRenderer is now separated from wrapping <li> component?,
thats something I deeply believe to be right approach

On HOC part:
I think withEvents is really terrible name, LI would make more sense.
As mentioned, whole point of HOC was the readability.

this is easier to read for me
const Day = LI(dayRenderer)
<Day ...props />

then this
<li onSelect={onSelect} ......>
<Day Renderer...props />
</li>
but I am ok with removing HOC.

On events part:
What do you mean by events, support for events out of the box?
My thinking of passing all props down to day renderer was to give user more options,
give him more freedom. In my experience more freedom, means more responsibility, but allows more simplicity.

I don't see how constraints would make the setup less complex, but I don't understand what you mean by event support out of the box probably.
For example my use case is parking places booking system where each day can be in one of three states booked, taken, free. How can InfiniteCalendar provide it out of the box.

@clauderic
Copy link
Owner

Hey @marek-sed, with regards to events, I'm working on supporting #6 out of the box, just because it's a frequent request and I'd like to tackle it properly.

With that in mind, I think the only props that would be relevant to pass down to the custom day renderer would be the date object, the events array associated to that date (if there are any) and whether or not that date isSelected. All click event listeners should be bound to the parent of the custom day renderer. In that context, the HOC becomes unnecessary.

@marek-sed
Copy link
Author

I think I understand your solution but, what I fear is what do you mean by out of the box, since you can't possibly know the events that will be used by user. So only way I can imagine to do that is to prescribe that user should pass a map of date, [event] pairs and dayRenderer to InfiniteCalendar, and in Month/Parent component you will just pick events from the map and pass them down to dayRenderer.
And maybe dayEvents would probably be better name then events. Since it can cause some confusion, if it is a js Event or calendar event.

So there is not much I can probabaly do with this pull request anymore, so good luck, and really looking forward to your solution :).

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

3 participants