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

New dayElement prop #89

Closed
gpbl opened this issue Nov 15, 2015 · 14 comments
Closed

New dayElement prop #89

gpbl opened this issue Nov 15, 2015 · 14 comments

Comments

@gpbl
Copy link
Owner

gpbl commented Nov 15, 2015

This prop should replace the current renderDay prop (that should become deprecated) and accept a React Element that will receive the day prop, e.g.

function MyCustomDay({ day }) {
  return <div>{ day.toString() }</div>;
}

function MyComponent() {
  return <DayPicker dayElement={ <MyCustomDay /> } />
}

(this will supersede #50)

@gpbl
Copy link
Owner Author

gpbl commented Mar 2, 2016

Closing – no need for this.

@olegakbarov
Copy link

Actually i was waiting for this feature 🙄

@gpbl
Copy link
Owner Author

gpbl commented Mar 3, 2016

@olegakbarov What is your use-case?

@olegakbarov
Copy link

I just need a custom element instead of renderDay.

@gpbl
Copy link
Owner Author

gpbl commented Mar 3, 2016

@olegakbarov what you need the custom element for, if I may ask?

@gpbl
Copy link
Owner Author

gpbl commented May 31, 2016

What about a dayClass prop? #172 (comment)

@lo1tuma
Copy link

lo1tuma commented Jun 13, 2016

I like this proposal. Currently we are using renderDay which depends on some state. In order to re-render, when the state has been changed, we need to create a new function which is bound to the new state and pass it to the DayPicker component. In order to avoid creating a new function each time when we render DayPicker it would be very helpful to define the day component class and additional props.

For Example:

class Day extends React.Component {
    render() {
        const isSelectable = this.props.date > this.props.selectableRangeStart && this.props.date < this.props.selectableRangeEnd;
        return <div className={ isSelectable ? 'selectable' : ''}>{this.props.date.getDate()}</div>;
    }
}
class DateRangePicker extends React.Component {
    render() {
         return <DayPicker dayClass={Day} dayProps={{ selectableRangeStart: this.state.selectableRangeStart, selectableRangeEnd: this.state.selectableRangeEnd }} />;
    }
}

@gpbl
Copy link
Owner Author

gpbl commented Jun 13, 2016

With the question raised by @boatkorachal and solved with #179 my conclusion is that we should go with dayElement. So, @lo1tuma your case could be:

<DayPicker 
    dayElement={ day => 
        <Day selectableRangeStart={this.state.selectableRangeStart} selectableRangeEnd={this.state.selectableRangeEnd} /> 
    } 
/>

Yet not sure about the signature of the dayElement function yet :)

What do you think?

@lo1tuma
Copy link

lo1tuma commented Jun 13, 2016

In your example dayElement would be be function returning an element, so I don’t see any difference to what we currently have with renderDay except that the name is different and more inaccurate. It also doesn’t eliminate the need for creating a new function each time when DayPicker should be (re-)rendered.

@gpbl
Copy link
Owner Author

gpbl commented Jun 13, 2016

In your example dayElement would be be function returning an element, so I don’t see any difference to what we currently have with renderDay

No, renderDay renders the content of the day cell, not the day cell itself. This new prop should render instead the day cell, as captionElement, navbarElement, etc. do.

It also doesn’t eliminate the need for creating a new function each time when DayPicker should be (re-)rendered.

If this is your issue, then you can do it also with renderDay:

const renderDayFactory = (start, end) => day => 
    <MyCustomDayContent day={day} selectableRangeStart={start} selectableRangeEnd={end} />

// later, in your render() method

<DayPicker renderDay={ renderDayFactory(this.state.selectableRangeStart, this.state.selectableRangeEnd) } />

Perhaps I don't understand your case?

@lo1tuma
Copy link

lo1tuma commented Jun 13, 2016

If this is your issue, then you can do it also with renderDay

That’s what we are currently doing. But I want to avoid creating new functions during rendering.

@gpbl
Copy link
Owner Author

gpbl commented Jun 13, 2016

@lo1tuma The issue is old and maybe its scope is not clear: basically the idea behind the dayElement prop was to use a different component instead of the current one. Seen the amount of logic in that component, I frankly don't see any use case for it.

Maybe it is possible to solve the problem you are rising with a different API. I don't understand why you are using renderDay and not the modifiers props:

<DayPicker
   modifiers={{
     selectable: day => day > this.state.selectableRangeStart && day < this.props.selectableRangeEnd;
   }}
/>

which adds already the selectable class to the day cell without the need of using renderDay.

@lo1tuma
Copy link

lo1tuma commented Jun 13, 2016

The issue is old and maybe its scope is not clear

Agreed, so basically my use case is to improve rendering performance. Initially we used modifiedr functions (4 of them) even with memoization. We render 12 months at once. On iOS (mobile safari) and old android devices rendering took serveral seconds (~2 to 5 seconds). Whenever the user interacts with our date range picker, we need the change the modifier functions (=> creating new functions) in order to trigger re-rendering of the DayPicker component, this means those 4 new functions have to be executed again for all 365 days. So every time to user clicks on a day, he has a 2 - 5 second delay until the UI reacts.
Then we switched to using renderDay where we can combine all 4 modifiers in a single function, which reduces the number of function calls from 1460 to 365 and a rendering time of 0.5 seconds. Which is better but still pretty slow.

@gpbl
Copy link
Owner Author

gpbl commented Jun 13, 2016

Sure, let's move the discussion here #181. Thanks for the numbers 👍

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

No branches or pull requests

3 participants