Migrate add/edit resources dialog to @material/web #21933
Conversation
|
Warning Rate limit exceeded@wendevlin has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 20 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughWalkthroughThe changes introduce a new custom dialog component, Changes
Assessment against linked issues
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (3)
src/panels/config/lovelace/resources/show-dialog-lovelace-resource-detail.ts (1)
Action Required: Update or Remove
removeResourceReferencesThe
removeResourcemethod is still being used insrc/panels/config/lovelace/resources/ha-config-lovelace-resources.ts. Please review and update the codebase to ensure all references to this method are consistent with the recent interface changes to prevent potential runtime errors.
- File:
src/panels/config/lovelace/resources/ha-config-lovelace-resources.ts
- Lines: References to
removeResourcemethod found.Analysis chain
Line range hint
3-10: Confirm removal ofremoveResourcemethod.The removal of the
removeResourcemethod from theLovelaceResourceDetailsDialogParamsinterface aligns with the PR's objectives. Ensure that all references to this method in other components are updated or removed to prevent runtime errors.Run the following script to verify the usage of
removeResource:Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that no other components are still using the removed `removeResource` method. # Test: Search for the method usage. Expect: No occurrences. rg --type typescript -A 5 $'removeResource'Length of output: 77
Script:
#!/bin/bash # Description: Verify that no other components are still using the removed `removeResource` method. # Test: Search for the method usage across all TypeScript files. Expect: No occurrences. rg 'removeResource' --glob '*.ts' -A 5Length of output: 1334
src/panels/config/lovelace/resources/ha-config-lovelace-resources.ts (2)
112-126: Approve the delete button configuration with a suggestion for improvement.The delete button configuration is well-implemented and aligns with the PR objectives. However, consider enhancing accessibility by adding an
aria-labelattribute to theha-icon-buttonfor better screen reader support.
256-295: Approve the resource deletion method with a suggestion for consistency.The
_removeResourcemethod is well-implemented, providing clear user confirmation steps and handling errors effectively. Consider usingasync/awaitconsistently in the catch block instead of returningfalsedirectly to handle potential asynchronous operations in error handling more robustly.
src/panels/config/lovelace/resources/dialog-lovelace-resource-detail.ts
Outdated
Show resolved
Hide resolved
src/panels/config/lovelace/resources/dialog-lovelace-resource-detail.ts
Outdated
Show resolved
Hide resolved
src/panels/config/lovelace/resources/ha-config-lovelace-resources.ts
Outdated
Show resolved
Hide resolved
src/panels/config/lovelace/resources/dialog-lovelace-resource-detail.ts
Outdated
Show resolved
Hide resolved
|
I've introduced the "xxx-new" components as first, and I've been regretting it since I did that. Why? I thought we could migrate way faster and then when all usages are away, use the old name again. However, until today, none of those introductions have been fully replaced and we have multiple "xxx-new" components now. People that start making frontend (knx, hacs, ...) based on this frontend start using these components since theme variables are defined in the component. Long story short, I would suggest introducing a "ha-dialog-m3" instead of "ha-dialog-new". |
|
The font Before
After
|
What is the issue with others using the new components? They are building their own packages and don't use our bundle, so it will not break if we migrate the names. If they bump frontend they will indeed have to rename the components too, but that is a simple search and replace? (On a separate note, I do hate the |
I just tried to explain that I do hate the "-new" components as well and suggest to make them somehow mention material 3. When material 4 will come, if ever, we will probably end up with "ha-dialog-new-new". 😆 Those that build frontends based on our frontend will have to change the name once, right now, and twice when we decide to go back to "ha-dialog". In some cases, it is not a search and replace, for example the button menu group. The dialog will be used much more common, so I would prefer "ha-m3-dialog" or "ha-dialog-m3", perhaps give those components an own folder (material 3) for every component where material 3 is adopted already. |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
src/panels/config/lovelace/resources/ha-config-lovelace-resources.ts (1)
255-293: LGTM, but consider improving error handling.The
_removeResourcemethod looks good overall. It handles the resource deletion process appropriately, including user confirmations and updating the internal state.However, the error handling could be improved. Instead of returning
falsein thecatchblock, consider showing an error dialog to the user with a more specific error message.Here's a suggestion to improve the error handling:
try { await deleteResource(this.hass!, resource.id); this._resources = this._resources!.filter(({ id }) => id !== resource.id); // ... } catch (err: any) { - return false; + showAlertDialog(this, { + title: this.hass!.localize( + "ui.panel.config.lovelace.resources.error_deleting_resource" + ), + text: err.message, + }); }Make sure to add the corresponding localization key for the error dialog title.
|
We should migrate everything to the new one before we start with But I get what you say, projects that have not updated the new ones will need to update ones we rename the new ones... I suggest |
|
@bramkragten @silamon I update the name to ha-md-dialog 👍 @bramkragten I did all of your requested changes except @piitaya I was able to set the line-height md variable, but there is no variable for letter-spacing. Should I keep it like it is or set the letter-spacing explicit for the ha-md-dialog? |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
src/panels/config/lovelace/resources/dialog-lovelace-resource-detail.ts (1)
99-107: Improve accessibility of the dialog header.The dialog header could benefit from additional accessibility enhancements:
- Add a
typeattribute to theha-dialog-headerelement to provide semantic meaning.- Add an
aria-labelattribute to theha-dialog-headerelement to provide a clear and concise label for screen readers.Here's an example of how you can improve the accessibility of the dialog header:
-<ha-dialog-header slot="headline"> +<ha-dialog-header slot="headline" type="heading" aria-label="Resource dialog header">
src/panels/config/lovelace/resources/dialog-lovelace-resource-detail.ts
Outdated
Show resolved
Hide resolved
…detail.ts Fix dialogClosed method naming Co-authored-by: Bram Kragten <mail@bramkragten.nl>
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
src/panels/config/lovelace/resources/dialog-lovelace-resource-detail.ts (1)
63-70: LGTM, but address the static analysis hint.The code changes are approved.
However, the static analysis tool reports that
_dialogClosedis declared but its value is never read. This is because_dialogClosedis used as an event handler for theclosedevent on theha-md-dialogcomponent, so it is not directly called in the code.To address the static analysis hint, add a comment to explain the purpose of
_dialogClosed:// Event handler for the `closed` event on the `ha-md-dialog` component. private _dialogClosed(): void { this._params = undefined; fireEvent(this, "dialog-closed", { dialog: this.localName }); }Tools
GitHub Check: Lint and check format
[failure] 63-63:
'_dialogClosed' is declared but its value is never read.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
src/panels/config/lovelace/resources/dialog-lovelace-resource-detail.ts (3)
46-47: Address the accessibility concerns.As mentioned in the previous review comment, consider adding a
typeattribute and anaria-labelto theha-md-dialogelement for better accessibility, since an element is used in theheadlineslot.
128-144: Ensure the cancel button is shown on all screen sizes.As discussed in the previous review comments, the cancel button should be:
- Shown on all screen sizes.
- Sticky at the bottom along with the submit button on mobile.
Please update the styles to ensure that the cancel button is always visible and positioned correctly on different screen sizes.
63-70: Consider swapping the public and private methods.As suggested in the previous review comment, consider making the
_dialogClosedmethod public and the current publiccloseDialogmethod private. This way, public close calls will trigger the necessary animations and ensure a consistent user experience.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
Files selected for processing (3)
- package.json (1 hunks)
- src/components/ha-md-dialog.ts (1 hunks)
- src/panels/config/lovelace/resources/dialog-lovelace-resource-detail.ts (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- package.json
- src/components/ha-md-dialog.ts
Additional comments not posted (10)
src/panels/config/lovelace/resources/dialog-lovelace-resource-detail.ts (10)
2-3: LGTM!The imports are correctly added and necessary for using Lit and decorators in the class.
5-9: LGTM!The imports are correctly added and necessary for using the new
ha-md-dialogcomponent and related functionality.
11-11: LGTM!The import is correctly added and necessary for using the
ha-icon-buttoncomponent.
46-47: LGTM!The private property
_dialogis correctly added using the@querydecorator and will be used to reference theha-md-dialogelement.
63-70: LGTM!The changes improve the code organization by:
- Making the
_dialogClosedmethod private and using it to handle the dialog closed logic.- Updating the public
closeDialogmethod to directly close the dialog.This refactoring enhances the clarity and maintainability of the code.
77-91: LGTM!The changes improve the accessibility and user experience by dynamically generating the dialog title and aria-label based on the resource parameters. The localization ensures that the strings are displayed in the user's preferred language.
93-106: LGTM!The changes update the dialog to use the new
ha-md-dialogcomponent and improve the dialog structure by:
- Adding a header with a close button and a dynamic title.
- Using slots for better composition and flexibility.
This enhances the user experience and aligns with the Material Design guidelines.
107-107: LGTM!The change improves the dialog structure by using a slot for the content. This allows for better composition and flexibility.
128-144: LGTM!The changes improve the dialog actions by:
- Adding a cancel button for better user experience.
- Updating the submit button text to reflect the action (create or update).
- Disabling the submit button based on the form state to prevent invalid submissions.
These enhancements align with the Material Design guidelines and provide a more intuitive user experience.
145-145: LGTM!The change is necessary to properly close the
ha-md-dialogelement.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/components/ha-md-dialog.ts (1 hunks)
Additional comments not posted (3)
src/components/ha-md-dialog.ts (3)
1-143: TheHaMdDialogcomponent implementation looks good overall!The component extends the
MdDialogfrom the Material Web library and introduces useful features like thedisableCancelActionproperty. The event handling and polyfill loading ensure compatibility across browsers, and the custom styles enhance the appearance and responsiveness of the dialog.
72-99: The_handleCancelmethod implementation looks good.The method correctly checks the
disableCancelActionproperty and prevents the default cancel action if needed. The rotation animation provides a nice visual feedback to the user.
35-37: Consider replacing theanimateAPI usage with CSS animations.As mentioned in the past review comments, the
animateAPI might not be supported by older browsers. It would be better to replace it with CSS animations for better browser support.Replace the
quickproperty usage with an explicit check for theanimateAPI support:-if (this.animate === undefined) { - this.quick = true; -} +this.quick = !this.animate;Likely invalid or redundant comment.
Breaking change
Proposed change
Type of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed:
Summary by CodeRabbit
New Features
HaMdDialog) that enhances user interaction with customizable behavior and improved visual design.disableCancelActionproperty to prevent dialog closure via escape key or outside click, enhancing user control.User Interface Enhancements