Fix: Display Issue of Calendar Icon in React Date Picker (#4291) #4298

franmc01 · 2023-10-09T15:56:40Z

🐛 Problem:

The calendar icon in the React Date Picker was not displaying as expected due to a conflict arising from a global * {box-sizing: border-box;} style, as discussed in issue #4291. Specifically, the icon container's padding interfered with its available space for content, causing the icon to not render visibly.

🛠 Solutions Considered:

Two main solutions were pondered:

Remove Padding: Adjust the position using left and right properties.
Change Box-Sizing: Explicitly set box-sizing: content-box; for the icon.

🚀 Adopted Solution:

The second solution was opted for in this PR due to its straightforwardness and minimal impact on existing style setups. By assigning box-sizing: content-box; to the icon, it assures the padding does not encroach on the icon’s space, thereby permitting it to be displayed even when a global box-sizing: border-box; is applied.

🧹 Additional Changes:

A small code adjustment was also introduced to enhance developer flexibility while interacting with the default SVG. Previously, resolving certain issues or introducing styles could inadvertently modify the base class, potentially causing unintended outcomes. The specific code change made is as follows:

- className="react-datepicker__calendar-icon"
+ className={`${defaultClass} ${className}`}

Here, defaultClass holds the essential CSS class for the calendar icon, whereas className can be used to inject additional classes for style customization or alteration without influencing the base. This affords developers greater control and flexibility over the icon’s appearance and behavior while ensuring the integrity of the default styling remains intact.

🧪 Test Adjustments:

In the process of resolving the primary issue, an adjustment was made to a test to ensure reliability and accuracy in its assertions. The change, as it pertains to the class name of the calendar icon, was as follows:

- expect(showIconClass).toBe("react-datepicker__calendar-icon");
+ expect(showIconClass).toMatch(/^react-datepicker__calendar-icon\s?$/);

This modification was brought into play due to the trailing whitespace within the class attribute, a nuance not addressed by the original test assertion. Two options were contemplated to manage this: employing .trim() to dismiss the whitespace, or utilizing a regex pattern to account for its potential presence.

The regex method was chosen for a few key reasons:

Clarity: It transparently accommodates the potential for a trailing space without masking it through string manipulation, maintaining a truer reflection of the DOM attribute’s state.
Robustness: It doesn’t change the string being tested, thereby reducing the chance of inadvertently altering or misrepresenting the attribute under examination.
Flexibility: It leaves room for accommodating variations in whitespace handling in future code adjustments without demanding test alterations.

Given the above, the chosen test modification provides a balanced blend of transparency, reliability, and foresight. Although .trim() could be utilized for a cleaner string comparison, we deemed the clarity in reflecting actual attribute values to be paramount, and thus, the regex approach was favored.

For future reference and clarity, the explanation is documented here in the PR. The incorporation of such a rationale directly in the codebase was considered. However, maintaining a lean code and accommodating these specifics in the PR/documentation might serve a dual purpose of avoiding inline clutter while still ensuring knowledge is shared and archived for posterity.

🎯 Outcome:

With these changes, the calendar icon will display consistently, regardless of global box-sizing styling, and developers are afforded enhanced flexibility concerning icon styling and customization.

🙏🏽 Request:

We kindly urge the maintainers to consider merging this PR promptly as it addresses a user-facing issue, ensuring enhanced usability and experience across implementations.

How to Test:

<DatePicker
  showIcon={true}
/>

Ensure that the calendar icon displays correctly even in environments where * {box-sizing: border-box;} is set.

Final Notes:

This fix aims to seamlessly integrate with existing implementations, ensuring no disruption while enhancing reliability in varied styling contexts. Your feedback and considerations for a swift merge would be immensely valued.

Additional Suggestion:

Moving forward, it might be beneficial to refine how we handle bug reports and feature requests in this repository. Enforcing a policy where issue creators provide a reproducible example, possibly through a StackBlitz or CodeSandbox setup, could streamline the debugging process. Having a readily available environment that mimics the reported issue will likely expedite the identification and resolution of potential defects, enabling us to maintain a robust and reliable codebase for all users.

We humbly propose this approach in the spirit of continuous improvement and welcome discussion on its potential implementation.

…version

pullrequest

✅ 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.

pullrequest

PullRequest Breakdown

Reviewable lines of change

+ 3
- 2

40% JavaScript
40% JavaScript (tests)
20% SCSS

Type of change

Fix - These changes are likely to be fixing a bug or issue.

package.json

martijnrusschen · 2023-10-09T15:59:14Z

src/calendar_icon.jsx

@@ -22,7 +22,7 @@ const CalendarIcon = ({ icon, className }) => {
  // Default SVG Icon
  return (
    <svg
-      className="react-datepicker__calendar-icon"
+      className={`${defaultClass} ${className}`}


Where do you define the default now?

Line 5, buddy! But let me check why it's failing because it was all green and working fine on my end before. I'll take a look now.

@martijnrusschen Found the issue

codecov · 2023-10-09T16:22:33Z

Codecov Report

Merging #4298 (ebd938a) into main (088e590) will not change coverage.
The diff coverage is n/a.

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

@@           Coverage Diff           @@
##             main    #4298   +/-   ##
=======================================
  Coverage   96.54%   96.54%           
=======================================
  Files          27       27           
  Lines        2374     2374           
  Branches      966      966           
=======================================
  Hits         2292     2292           
  Misses         82       82

Files	Coverage Δ
src/calendar_icon.jsx	`100.00% <ø> (ø)`

pullrequest

This is a pretty clean fix with a PR great description! For the sake of maintainability, the test change should be explained in a comment (or the test change could be obviated, see comment for details).

Jim K

Reviewed with ❤️ by PullRequest

pullrequest · 2023-10-09T16:41:31Z

test/datepicker_test.test.js

@@ -2234,7 +2234,7 @@ describe("DatePicker", () => {
      datePicker,
      "react-datepicker__calendar-icon",
    ).getAttribute("class");
-    expect(showIconClass).toBe("react-datepicker__calendar-icon");
+    expect(showIconClass).toMatch(/^react-datepicker__calendar-icon\s?$/);


A comment explaining that there is a trailing space here because the code puts one in to allow for a second class (not used in this test) will help someone looking at this in the future and wondering what this is about.

Alternatively, this test change could be reverted and the code could just make sure there's no trailing white space in the attribute value.

🔸 Improve Readability (Important)

Jim K

Appreciate your insights! 🚀 I opted for expect(showIconClass).toMatch(/^react-datepicker__calendar-icon\s?$/) over using trim() because it directly allows for potential, intentional trailing spaces, signaling an openness for additional class names. It kind of sets a clear expectation, without assuming unnecessary whitespaces.

In regards to in-code comments, I believe this PR and the associated discussion provide a decent historical record for future code wanderers. However, if inline comments are the norm here, happy to adapt! 😄

martijnrusschen · 2023-10-09T16:46:19Z

src/calendar_icon.jsx

@@ -22,7 +22,7 @@ const CalendarIcon = ({ icon, className }) => {
  // Default SVG Icon
  return (
    <svg
-      className="react-datepicker__calendar-icon"
+      className={`${defaultClass} ${className}`}


Can you explain why this change was needed? In the PR description you explained how the CSS change was needed, but this seems unrelated.

@martijnrusschen
Thank you for pointing that out! 🙌 I've gone ahead and updated the PR description to encompass all the adjustments and their respective rationales. Apologies for any confusion caused and I appreciate your vigilance in ensuring all changes are accurately documented and justified! 🚀

Thanks again for your time and keen eye! 🧐🚀

franmc01 added 2 commits October 9, 2023 13:32

fix: resolve issue Hacker0x01#4291

e765340

feat: Add ability to add styles to SVG by default and update package …

914354a

…version

pullrequest bot reviewed Oct 9, 2023

View reviewed changes

martijnrusschen requested changes Oct 9, 2023

View reviewed changes

fix: 🔄 revert package version and 🚑 adjust failing test

bcc1dc3

pullrequest bot reviewed Oct 9, 2023

View reviewed changes

martijnrusschen reviewed Oct 9, 2023

View reviewed changes

martijnrusschen approved these changes Oct 9, 2023

View reviewed changes

martijnrusschen merged commit 2122df4 into Hacker0x01:main Oct 9, 2023
4 checks passed

snyk-io bot mentioned this pull request Jun 11, 2024

[Snyk] Upgrade react-datepicker from 4.11.0 to 4.25.0 Opetushallitus/rekisterointi#30

Closed

jones58 mentioned this pull request Aug 6, 2024

[Snyk] Upgrade react-datepicker from 4.8.0 to 4.25.0 jones58/personal-knowledge-wiki#29

Merged

rowidanagah mentioned this pull request Aug 25, 2024

[Snyk] Upgrade react-datepicker from 4.11.0 to 4.25.0 rowidanagah/EduMentorMate#9

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Display Issue of Calendar Icon in React Date Picker (#4291) #4298

Fix: Display Issue of Calendar Icon in React Date Picker (#4291) #4298

franmc01 commented Oct 9, 2023 •

edited

Loading

pullrequest bot left a comment

pullrequest bot left a comment •

edited

Loading

martijnrusschen Oct 9, 2023

franmc01 Oct 9, 2023 •

edited

Loading

franmc01 Oct 9, 2023

codecov bot commented Oct 9, 2023

pullrequest bot left a comment

pullrequest bot Oct 9, 2023

franmc01 Oct 9, 2023

martijnrusschen Oct 9, 2023

franmc01 Oct 9, 2023

Fix: Display Issue of Calendar Icon in React Date Picker (#4291) #4298

Fix: Display Issue of Calendar Icon in React Date Picker (#4291) #4298

Conversation

franmc01 commented Oct 9, 2023 • edited Loading

🐛 Problem:

🛠 Solutions Considered:

🚀 Adopted Solution:

🧹 Additional Changes:

🧪 Test Adjustments:

🎯 Outcome:

🙏🏽 Request:

How to Test:

Final Notes:

Additional Suggestion:

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot left a comment • edited Loading

Choose a reason for hiding this comment

PullRequest Breakdown

martijnrusschen Oct 9, 2023

Choose a reason for hiding this comment

franmc01 Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

franmc01 Oct 9, 2023

Choose a reason for hiding this comment

codecov bot commented Oct 9, 2023

Codecov Report

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot Oct 9, 2023

Choose a reason for hiding this comment

🔸 Improve Readability (Important)

franmc01 Oct 9, 2023

Choose a reason for hiding this comment

martijnrusschen Oct 9, 2023

Choose a reason for hiding this comment

franmc01 Oct 9, 2023

Choose a reason for hiding this comment

franmc01 commented Oct 9, 2023 •

edited

Loading

pullrequest bot left a comment •

edited

Loading

franmc01 Oct 9, 2023 •

edited

Loading