-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Clearable time selector #18590
Clearable time selector #18590
Conversation
fe38238
to
882542a
Compare
e6f6f32
to
a893222
Compare
7cae4f4
to
45845b4
Compare
45845b4
to
6bbd70c
Compare
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.
LGTM
Once we get this would be good to add for |
6bbd70c
to
3f1ec34
Compare
I think this will solve the issue where its not possible to clear a timeout without dropping back to YAML once its been set which seems like the root cause of home-assistant/core#109586 It also resolves quandary in home-assistant/core#115830 as it means it means we align the frontend to the backend
|
WalkthroughWalkthroughThe changes introduce a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HaTimeInput
participant HaBaseTimeInput
participant HaTimeSelector
User->>HaTimeInput: Trigger clear button
HaTimeInput->>HaBaseTimeInput: Set value to undefined
HaBaseTimeInput->>HaTimeSelector: Clear value
HaTimeSelector-->>HaBaseTimeInput: Value cleared
HaBaseTimeInput-->>HaTimeInput: Value cleared
HaTimeInput-->>User: Value cleared
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
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.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (2)
src/components/ha-time-input.ts (1)
66-67
: Clarify the comment foreventValue
being undefined.The comment indicates that an undefined
eventValue
means the time selector is being cleared. Ensure that this is the intended behavior and that it is clearly documented.- // An undefined eventValue means the time selector is being cleared, + // An undefined eventValue means the time selector is being cleared. + // Ensure this behavior is clearly documented.src/components/ha-base-time-input.ts (1)
270-272
: Clarify the_clearValue
method.The
_clearValue
method triggers avalue-changed
event. Ensure that this behavior is clearly documented and that the method name reflects its functionality.- private _clearValue(): void { - fireEvent(this, "value-changed"); + /** + * Clears the input value and triggers a value-changed event. + */ + private _clearValue(): void { + fireEvent(this, "value-changed", { value: undefined });
(!isNaN(eventValue.hours) || | ||
!isNaN(eventValue.minutes) || | ||
!isNaN(eventValue.seconds)) |
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.
Use Number.isNaN
instead of isNaN
.
isNaN
is unsafe as it attempts a type coercion. Use Number.isNaN
instead for better type safety.
- (!isNaN(eventValue.hours) ||
- !isNaN(eventValue.minutes) ||
- !isNaN(eventValue.seconds))
+ (!Number.isNaN(eventValue.hours) ||
+ !Number.isNaN(eventValue.minutes) ||
+ !Number.isNaN(eventValue.seconds))
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
(!isNaN(eventValue.hours) || | |
!isNaN(eventValue.minutes) || | |
!isNaN(eventValue.seconds)) | |
(!Number.isNaN(eventValue.hours) || | |
!Number.isNaN(eventValue.minutes) || | |
!Number.isNaN(eventValue.seconds)) |
Tools
Biome
[error] 70-70: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
[error] 71-71: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
[error] 72-72: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
@matthiasdebaat |
YOU ROCK! Thanks! |
@matthiasdebaat , the current ux is good to go? |
One question, before it's good to go. Does this field have a adjacent label? Or do you need to add one above by yourself? |
Co-authored-by: Bram Kragten <[email protected]>
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.
Actionable comments posted: 1
// An undefined eventValue means the time selector is being cleared, | ||
// the `value` variable will (intentionally) be left undefined. | ||
if ( | ||
!isNaN(eventValue.hours) || | ||
!isNaN(eventValue.minutes) || | ||
!isNaN(eventValue.seconds) | ||
eventValue !== undefined && | ||
(!isNaN(eventValue.hours) || | ||
!isNaN(eventValue.minutes) || | ||
!isNaN(eventValue.seconds)) |
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.
Use Number.isNaN
instead of isNaN
.
isNaN
is unsafe as it attempts a type coercion. Use Number.isNaN
instead for better type safety.
- (!isNaN(eventValue.hours) ||
- !isNaN(eventValue.minutes) ||
- !isNaN(eventValue.seconds))
+ (!Number.isNaN(eventValue.hours) ||
+ !Number.isNaN(eventValue.minutes) ||
+ !Number.isNaN(eventValue.seconds))
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
// An undefined eventValue means the time selector is being cleared, | |
// the `value` variable will (intentionally) be left undefined. | |
if ( | |
!isNaN(eventValue.hours) || | |
!isNaN(eventValue.minutes) || | |
!isNaN(eventValue.seconds) | |
eventValue !== undefined && | |
(!isNaN(eventValue.hours) || | |
!isNaN(eventValue.minutes) || | |
!isNaN(eventValue.seconds)) | |
// An undefined eventValue means the time selector is being cleared, | |
// the `value` variable will (intentionally) be left undefined. | |
if ( | |
eventValue !== undefined && | |
(!Number.isNaN(eventValue.hours) || | |
!Number.isNaN(eventValue.minutes) || | |
!Number.isNaN(eventValue.seconds)) |
Tools
Biome
[error] 70-70: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
[error] 71-71: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
[error] 72-72: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
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.
These assignments should be translatable.
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Whenever you're ready, feel free to hit the "ready to review" button. 🥇 |
Proposed change
The time selector can now be cleared:
The same selector after pressing the clear button:
Type of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed:
Summary by CodeRabbit
New Features
clearable
,required
, anddisabled
).Enhancements
clearable
property for improved user control over input fields, including visual and functional updates to accommodate the new feature.