-
Notifications
You must be signed in to change notification settings - Fork 13
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
Add a Month picker & other fixes #1510
Add a Month picker & other fixes #1510
Conversation
cae4b64
to
89806c0
Compare
src/DatePickers/DatePicker.tsx
Outdated
|
||
const DatePicker = forwardRef<unknown, DatePickerProps>(({ highlightDates, ...props }, datePickerRef) => { | ||
const handleDownKey = () => { | ||
console.log("inside the date picker"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget to remove this
}: ReactDatePickerCustomHeaderProps & { locale?: string }) { | ||
const { t } = useTranslation(); | ||
|
||
console.log(t("go to previous year")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one too
}: ReactDatePickerCustomHeaderProps & { locale?: string }) { | ||
const { t } = useTranslation(); | ||
|
||
console.log(t("go to previous year")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more
placeholder: (inputProps && inputProps.placeholder) || defaultPlaceholder, | ||
}; | ||
|
||
const customInput = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be defined outside of the component body?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it's valid to define elements inside of a component body so long as they need access to props/state. It's only components that shall not be defined inside components as far as I understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could have sworn there was a React Rule about this, but I cannot find it. I thought the challenge with inline definitions like this was that you effectively recreate the component on each re-render?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's correct in the case of a component (function), however, I've seen many libraries defining elements (constants) inside component bodies.
I believe the issue with components inside components is that the state will be flushed at every re-render which can be problematic.
Edit: Actually I am not sure about this anymore. ChatGPT thinks both are ok, which I am 99% sure is not the case:
https://chatgpt.com/share/6791459f-ddd8-800a-859e-d12bad980a08
I'm a little worried about those unknowns floating around here. Can they be narrowed at all? |
96f35e2
to
9edc3ac
Compare
We can narrow it down and I did in the most recent commit. I am worried that it might create breaking changes for people since it was unknown in the past. I am trying to balance correctness with backwards compatibility. |
a696672
to
bb1ccfe
Compare
🎉 This PR is included in version 13.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
ts-nocheck
from the DateRange componentChanges include
Feature checklist