Skip to content

docs: fix custom components docs #2668

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

Merged
merged 1 commit into from
Jan 19, 2025
Merged

Conversation

rodgobbi
Copy link
Collaborator

What's Changed

  • Fix Custom Components docs.
  • Update CustomDayButton example to reflect documentation changes.

The documentation gives examples of customization based on a render props API, but the customization available is through a component-based API.
E.g. the DayButton component is rendered as <components.DayButton {...} /> in the DayPicker. To follow a render props API style, it should be manually called in the DayPicker component like components.DayButton({...}).
Using the components prop by passing inline functions for the custom components can cause issues like those reported in #2667.
This happens because on every render a new function reference is created and mounted as a React component, the React's reconciliation algorithm considers that a new component has been rendered, and because of that it unmounts the previous component and mounts the new one.
This can cause performance issues and also bugs with state resetting because the component is unmounted and then mounted again, resetting the state to the initial values.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Update CustomDayButton example to reflect documentation changes
Copy link
Owner

@gpbl gpbl left a comment

Choose a reason for hiding this comment

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

Thanks for your PR!

Actually, a function that takes React props as arguments is still a functional component. I don’t see any conceptual difference between declaring it directly in the components prop (as in the example online) and declaring it in the constant above.

(one issue could be the performance while creating the component at each render, so the code there doesn't fit for a good example)

I’m not sure either if the current architecture supports class components. This could be an interesting area to investigate.


We definitely need more practical examples and better documentation related to custom components.

  • Using custom components with external UI libraries like react-bootstrap or MUI.
  • Enhancing the content of the day cell (e.g. with a Tooltip)
  • Implementing a Year navigator who clicking on the Month Caption
  • ...

We should collect some “use cases” by searching through this repo’s discussions. Or finding open-source DayPicker implementations that use custom components.

While working on some examples, we could easily identify the parts that are less developer-friendly and adjust DayPicker to be simpler to use in those cases.

In conclusion, to me this PR still doesn't address the core problems in the custom component docs. If you have some time, open an issue where we can focus on “Improving custom component docs.”

Thanks!

function DayButton(props: DayButtonProps) {
const { day, modifiers, ...buttonProps } = props;

const { setSelected } = React.use(SelectedDateContext);
Copy link
Owner

Choose a reason for hiding this comment

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

There should be no need to use a new context to store the selected state. DayPicker already runs inside a similar context, which is accessible through the DayPicker hook.

Suggested change
const { setSelected } = React.use(SelectedDateContext);
const { select } = useDayPicker();

Copy link
Collaborator Author

@rodgobbi rodgobbi Jan 17, 2025

Choose a reason for hiding this comment

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

I tried to use it, but we have some caveats to deal with it:
image

It doesn't accept undefined as an argument for the Date.
Also each select based on the mode has specific logic related to them that the consumers may not be aware.
E.g. the select logic for single day mode: https://github.com/gpbl/react-day-picker/blob/main/src/selection/useSingle.tsx

Let me know if we should rethink this example.
I tried to keep the same logic in this PR and focus on the docs update, that's why I avoided changing a lot the example.

@rodgobbi
Copy link
Collaborator Author

Actually, a function that takes React props as arguments is still a functional component. I don’t see any conceptual difference between declaring it directly in the components prop (as in the example online) and declaring it in the constant above.

There is a difference, actually. I've also confused this in the past because both render props and React components return JSX / React elements, but they are not interchangeable in the way they are used, even though in certain use cases changing one with the other doesn't break the application, it still can potentially cause issues at some point, like #2667, that's why I think this PR is still valid.
I'll elaborate further to clarify the difference and argue why it would be better to update the docs.

Starting with the similarity mentioned above, both render props and React components return JSX / React elements, their output is the same, but the difference is how they are called.

A render props is always called in user land, meaning always called by user code, mainly inside React components, like in the example in the link of the React docs, if we have a renderContent props, in the React component code it would be directly called like renderContent(...).
The parameters for a render prop can be anything, including the same as React component, because the render prop is not passed to the React engine.
And because the render prop is called by user code and not the React engine, we cannot use hooks in it, it's just a function that returns React elements, that's why its naming doesn't follow the convention for React components of using CamelCase.

A React component should never be called in user land and only by the React engine, this is what allows components to use React hooks and React manage hook state.
That's the case of the DayPicker's components, as the DayPicker mounts them as JSX like <components.DayButton {...} />. If any of them uses React hooks, it would throw an error if trying to use them as render prop calling it like components.DayButton({...}).

There are use cases where misusing them doesn't directly throw an error, but can eventually cause issues like #2667, that's why I think updating the docs to reflect the correct usage would be better.
My team faced an issue in the past when using the component the way the docs recommend that was caused because of the React component lifecycle of unmounting and mounting resets the component state, that's why I also mentioned it in the PR description.
For reference, another example of misuse that works, we can directly call a functional component in the code if it doesn't have any React hook, but as soon as a hook is added, React will throw an error.

I’m not sure either if the current architecture supports class components. This could be an interesting area to investigate.

It should work, it's just not possible to use hooks in a class component, but this can be worked around by having wrapper function component that uses the day picker hooks and pass the data to a class component.

We definitely need more practical examples and better documentation related to custom components.

That would be great indeed, I can work on that.
Also, that's one the reasons why I used React context in the example I changed.
For context, not all developers are familiar with React context usage, I helped colleagues in the past to solve a problem of passing data to DayPicker custom components by guiding them to use React context. It's related to the same issue I mentioned above, where we could not use inline render functions for custom components, because some custom components had internal state, and the state would get reset on every rerender, but we also needed to pass props not controlled by the DayPicker to the custom components.

Copy link
Owner

@gpbl gpbl left a comment

Choose a reason for hiding this comment

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

I'd be interested to know if we can remove the React Context and use the useDayPicker hook instead. If it works, then better use it in the examples you are changing here.

If there are potential issues creating a new component as our code sample suggest, then I do agree to not suggest that approach. We should change also this example in the markdown to something like this:

function CustomDayCell(props: DayProps) {
	...
}
function CustomMonthGrid(props: DayProps) {
	...
}
<DayPicker
  components={{
    Day: CustomDayCell,
    MonthGrid: CustomMonthGrid,
  }}
/>
  • To me we lack some meaningful example demonstrating how to use a custom component.
    • Examples should show use cases that are easy to understand, and suggest potential applications for a custom components,
    • The styled components used in Custom Component too many rerender #2667 are a good example too!

@gpbl gpbl self-requested a review January 17, 2025 00:15
@gpbl
Copy link
Owner

gpbl commented Jan 17, 2025

I could check out better your change and I see now the reason to use a context there… but something is missing if we require users to use a context in this manner.

Do you think the hook is missing a setSelected function in its return value? 🤔

@rodgobbi
Copy link
Collaborator Author

I'd be interested to know if we can remove the React Context and use the useDayPicker hook instead. If it works, then better use it in the examples you are changing here.

My example wasn't the best one to showcase the need for the React context, sorry about that.
I'll elaborate more on why consumers may need a React context with the current API, and another potential API that can be added to solve the issue, and we can discuss in parallel if the DayPicker is missing providing the setSelected function through the React context.

The example of passing setSelect through context is not great because it is already managed by the DayPicker. However, if a consumer needs to pass custom props that the DayPicker does not have context for, the consumer needs to use React context with the current API.
The code in #2667 has a use case that serves as a good example for discussion.
In the code example, the imgSrc passed to the CalendarCustomDay is computed from the products data, which is outside the scope of the DayPicker.
After updating the code with my suggestion, the CalendarCustomDay passed to the components will look like:

        components={{
          // ....
          Day: CalendarCustomDay,
        }}

Therefore, there's no direct way of passing the imgSrc prop, and neither the getProductImage function nor the products data, to be able to compute the imgSrc based on the day.date passed to the components.Day component.

The only way to solve this that I can think of using the current API is to create a React context.
To avoid the need for a React context, the DayPicker could expose an extra API just for passing through custom props to the custom components.
For reference, Material UI has such API, called componentsProps.
(it's now deprecated due to naming changes, but the DayPicker has a components prop so it makes sense to name it componentsProps)

I could check out better your change and I see now the reason to use a context there… but something is missing if we require users to use a context in this manner.

I'll try to use the DayPicker hooks for the current example in this PR, as it is helpful to show how to use the hooks provided by the lib.

But it would also be great to have examples passing custom props that are not managed by the DayPicker, which is the use case I discussed above, and I can only think of using React context to solve it.
Let me know your thoughts, and if the componentsProps API mentioned above makes sense and would be helpful to avoid the need for React contexts.
I can follow up on this in another PR.

Copy link
Owner

@gpbl gpbl left a comment

Choose a reason for hiding this comment

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

I understood your points... and see how it’s challenging to select days from custom components.

I believe this is a good change and an improvement over the current docs. I appreciate your efforts and patience here! thanks!

@gpbl gpbl merged commit bf909c2 into gpbl:main Jan 19, 2025
11 checks passed
@rodgobbi
Copy link
Collaborator Author

You're welcome and thanks @gpbl !

We should collect some “use cases” by searching through this repo’s discussions. Or finding open-source DayPicker implementations that use custom components.

I'll follow your suggestion and look for examples to use as references to add to the docs, I'll open a PR later with more examples.
The examples you gave are also good.
I think the example https://date-picker.luca-felix.com/ you mentioned in another discussion implements the "Implementing a Year navigator who clicking on the Month Caption" example, doesn't it?

Let me know if you have an opinion about adding a componentsProps API like MUI to the DayPicker.
I can create a PR as PoC to be able to visualize better and discuss it.

@gpbl
Copy link
Owner

gpbl commented Jan 19, 2025

Yes, date-picker.luca-felix.com is an excellent example of extending DayPicker. We could also implement the years navigation feature, which has been requested a few times. However, our primary focus should be on making DayPicker easier and more intuitive to extend.

I'm not sure if the componentsProps is a pattern useful for DayPicker - what are the main goals of custom components in DayPicker, and how could we improve how they have been used so far by other devs? What about the useDayPicker hook and the related context? 🤔

@rodgobbi
Copy link
Collaborator Author

@gpbl
I created a CodeSandbox example to visualize better how it would look. (I should have done this sooner 😄 )

The code sandbox has an example of a state that is not managed by the DayPicker, so using its hooks is not enough, as it doesn't have any context for the things defined by the parent components.
The first example works because it is using the API correctly, and the second one follows the example of the previous version of the docs, and the state of the custom Day component gets reset for the reasons I mentioned before.

To make the first example work we inevitably need to create a React context, the componentsProps would be a way to avoid the need for that.
But as I wrote the example and wrote a comment on how it would look with the componetsProps prop, I didn't see much benefit from having the new API.
It's a matter of taste and if it's worth getting rid of the need for a React context for such use cases.
In any case, it's good to write the code sandbox example to be able to visualize and discuss it.

@gpbl
Copy link
Owner

gpbl commented Jan 20, 2025

Interesting pattern indeed, and I like how you could expose the issue when passing components that way.

What turns me off about componentsProps is that it’s not very common (AFAIK), which could alienate users, and it’s not as declarative as we’re used to work in React. Do we actually need to introduce this pattern?

Maybe there's a way to pass stricter types: i.e. forbid passing a new function as custom components:

components={
  Day: () => <div/> // ❌ typecheck fails
  Day: CustomDay // ✅ typecheck OK
}	

For non TypeScript users, we could explain this better in the docs. Would this work?

Re: enhancing the user experience of custom components, let’s start creating a some examples that can help users understand how custom components work. (not a priority)

@rodgobbi
Copy link
Collaborator Author

What turns me off about componentsProps is that it’s not very common (AFAIK), which could alienate users, and it’s not as declarative as we’re used to work in React. Do we actually need to introduce this pattern?

It's not that uncommon for component-based API, but nowadays it's more common the render props API style which already solves the issue of passing down state and props coming from the parent component.
But after seeing more of it I agree that it doesn't seem worth it.
We can check later and evaluate this again if we see a lot of people mentioning if there's a way to avoid needing a React context for their use case. For now, this looks like an uncommon use case, which fortunately has a solution in any case.

Maybe there's a way to pass stricter types: i.e. forbid passing a new function as custom components:

I think there's no way of enforcing this with TS, both cases are functions that return React elements, and due to TS duck typing they are the same type.
I think this could only be enforced with a linter rule, much like eslint-plugin-react-hooks.

For now, I can only think of warning the users in the docs about how the API should be used, and having an example using React context to provide custom props to any custom components if needed.
What do you think?

Re: enhancing the user experience of custom components, let’s start creating a some examples that can help users understand how custom components work. (not a priority)

I'll try to help with that later 👍
(After I check if we can set sideEffects to false)

@gpbl
Copy link
Owner

gpbl commented Jan 22, 2025

Aside: examples using Tailwind and DayPicker: https://originui.com/calendars-date-pickers

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