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

feat: add customizable calendar icon #4295

Merged

Conversation

franmc01
Copy link
Contributor

@franmc01 franmc01 commented Oct 7, 2023

🚀 Proposal:

Enhance the customization capabilities of the calendar icon in the DatePicker component, allowing the use of custom icons and not solely the default provided icon.

🏗 Changes Made:

  1. New icon Property:

    • Introduced an icon prop that accepts either a string (icon class name) or a React component (e.g., a custom SVG).
    • If icon is a string, an <i> element is rendered with that string as its class name.
    • If icon is a React component, it is rendered as-is.
  2. New calendarIconClassName Property:

    • Introduced an additional calendarIconClassName prop for further icon styling customization.
    • The value of calendarIconClassName will be added as an additional CSS class to the calendar icon, allowing enhanced style customization.
  3. Code Refactor:

    • Extracted the calendar icon rendering into a separate component (CalendarIcon) for improved code readability and maintainability.
    • Updated the rendering logic in the renderInputContainer method to utilize the new CalendarIcon component.

🎯 Motivation:

The inspiration behind these changes is to bestow developers with greater control over the DatePicker's display. By enabling calendar icon customization, we enhance the developers' UI control over the DatePicker without sacrificing functionality or user experience.

👀 How to Use the New Feature:

You can now utilize various renowned icon libraries, such as Font Awesome, Material Icons, or your custom SVGs, with the new icon prop. Here are examples of how to implement this:

// Using an icon class name from Font Awesome.
<DatePicker
  showIcon={true}
  icon="fa fa-calendar-alt"
/>

// Using an icon class name from Material Icons.
<DatePicker
  showIcon={true}
  icon="material-icons calendar-icon"
/>

// Using a React component or custom SVG.
import MyCustomIcon from './path-to-icons/MyCustomIcon';

<DatePicker
  showIcon={true}
  icon={<MyCustomIcon />}
/>

🔄 Backward Compatibility:

It is pivotal to note that these changes have been implemented to ensure backward compatibility. Existing implementations of the DatePicker using the showIcon prop will continue to function as before, displaying the default calendar icon if no icon prop is provided.


📝 Final Notes:

These proposed changes aim to deliver a flexible solution to accommodate various needs without introducing significant overhead to the codebase or altering the API properties in a way that disrupts existing implementations. I am open to feedback and suggestions to refine this proposal and hope that this change brings valuable additions to the react-datepicker user community.


Introduce the ability to customize the calendar icon used in the DatePicker. This is achieved by allowing a new  prop which can accept a string (class name) or a React component. This aims to enhance the component's flexibility and user customization options.
Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

✅ This pull request was sent to the PullRequest network.


@frankops11 you can click here to see the review status or cancel the code review job.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

PullRequest Breakdown

Reviewable lines of change

+ 172
- 10

60% JavaScript
34% JavaScript (tests)
4% Markdown
2% HTML

Type of change

Feature - These changes are adding a new feature or improvement to existing code.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

It might be worth adding an example for the second use case, but outside of that this looks like a solid feature to add and I see no issues with the implementation to prevent this from moving forward

Image of James D James D


Reviewed with ❤️ by PullRequest

@@ -5,6 +5,7 @@
showIcon
selected={startDate}
onChange={(date) => setStartDate(date)}
icon="fa fa-calendar"
Copy link

Choose a reason for hiding this comment

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

Would it be worth adding an example for an SVG React component as well?

🔹 Style Consistency (Nice to have)

Image of James D James D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

James, good call on the SVG component example. I'll add that in and push it soon. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2023-10-06 at 22 52 16

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi James,

I've added the examples! 🚀 The SVG one isn’t as clean since I embedded the SVG directly, but I hope it makes sense. :D

Cheers!

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

This looks really useful, and thanks for writing so much in the PR description.

The only comment I really have for improvement is it could be nice to have some tests for this.

Image of Dallas Dallas


Reviewed with ❤️ by PullRequest

@franmc01
Copy link
Contributor Author

franmc01 commented Oct 7, 2023

@martijnrusschen

This looks really useful, and thanks for writing so much in the PR description.

The only comment I really have for improvement is it could be nice to have some tests for this.

Image of Dallas Dallas

Reviewed with ❤️ by PullRequest

Hi Dallas,

Absolutely! I've attached a few, looking forward to your feedback! 😄

Screenshot 2023-10-06 at 22 54 04

Cheers!

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

PullRequest reviewed the updates made to #4295 up until the latest commit (6c502ed). No further issues were found.

Reviewed by:

Image of Dallas Dallas

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

@martijnrusschen

This looks really useful, and thanks for writing so much in the PR description.

The only comment I really have for improvement is it could be nice to have some tests for this.

Reviewed with ❤️ by PullRequest

Hi Dallas,

Absolutely! I've attached a few, looking forward to your feedback! 😄

Screenshot 2023-10-06 at 22 54 04

Cheers!

Thanks!! These are great.

Image of Dallas Dallas

@codecov
Copy link

codecov bot commented Oct 7, 2023

Codecov Report

Merging #4295 (318be85) into main (27788f5) will increase coverage by 0.01%.
Report is 12 commits behind head on main.
The diff coverage is 100.00%.

❗ Current head 318be85 differs from pull request most recent head dedb21b. Consider uploading reports for the commit dedb21b to get more accurate results

@@            Coverage Diff             @@
##             main    #4295      +/-   ##
==========================================
+ Coverage   96.52%   96.54%   +0.01%     
==========================================
  Files          25       27       +2     
  Lines        2358     2370      +12     
  Branches      962      965       +3     
==========================================
+ Hits         2276     2288      +12     
  Misses         82       82              
Files Coverage Δ
src/calendar_icon.jsx 100.00% <100.00%> (ø)
src/index.jsx 94.24% <100.00%> (ø)
test/helper_components/calendar_icon.jsx 100.00% <100.00%> (ø)

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

PullRequest reviewed the updates made to #4295 up until the latest commit (153c87b). No further issues were found.

Reviewed by:

Image of Dallas Dallas

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

PullRequest reviewed the updates made to #4295 up until the latest commit (dedb21b). No further issues were found.

Reviewed by:

Image of Dallas Dallas

@franmc01
Copy link
Contributor Author

franmc01 commented Oct 8, 2023

Hey @martijnrusschen!

I'm Francisco, and I've been poking around and contributing a bit to react-datepicker lately. Just shot a PR your way with a feature that allows for more versatile calendar icon customization. Have you had a chance to take a look? I’d love to hear your thoughts and find out if there’s a chance we could merge it into main.

Along the way, I've also been thinking about a couple of things that could be cool for the project. For instance, I'm considering playing around with VitePress to propose a redesign of the documentation website. It’s not bad as it stands, but I’ve got some ideas that might make it a bit more polished and user-friendly. Would you be open to discussing this? I'm keen on continuing to contribute to the project, both in improvements and solving issues.

Looking forward to your feedback on the PR and chatting about these ideas.

Cheers!
Francisco

Copy link
Member

@martijnrusschen martijnrusschen left a comment

Choose a reason for hiding this comment

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

Thank you for the extensive examples and write-up. This is looking good!

@martijnrusschen martijnrusschen merged commit fa3eb51 into Hacker0x01:main Oct 8, 2023
4 checks passed
@martijnrusschen
Copy link
Member

martijnrusschen commented Oct 8, 2023

@frankops11 Thanks for your contribution. I'd love to hear your thoughts on improving the documentation site. The component has significantly grown over the years and could use some rework to make it future proof.

@franmc01
Copy link
Contributor Author

franmc01 commented Oct 8, 2023

@frankops11 Thanks for your contribution. I'd love to hear your thoughts on improving the documentation site. The component has significantly grown over the years and could use some rework to make it future proof.

Hey, thanks a lot for merging the PR and for being open to dialogue about improving the documentation site!

I'm particularly excited about infusing the documentation site with a modern, sleek aesthetic, and VitePress strikes me as an ideal tool for this. Despite its Vue.js foundation, we can utilize Whyframe to render React previews and examples. This would not only allow users to explore live demos but also interact with and understand the code in an approachable manner.

Diving into react-datepicker component enhancements:

TypeScript Compatibility: Generating or exposing TypeScript definitions should enhance the experience for TypeScript projects. A full TypeScript migration is in my thoughts too but let's indeed tread step by step.
Enhanced Customization: Implementing a root of CSS variables to facilitate user customization is another pivotal step forward. Drawing inspiration from the Swiper library and its philosophy, we can provide a quick and intuitive customization path, enhancing user experience while preserving functionality.
All of these considerations and potential improvements remain faithfully within the aesthetic and professional identity of HackerOne, ensuring that react-datepicker continues to flourish both in usability and community engagement.

Eager to dive into your thoughts and, upon approval, immerse myself in implementing these enhancements!

Warm regards,
Francisco

@martijnrusschen
Copy link
Member

Let's move this discussion to an issue or discussion so we can chat about the initiatives.

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