-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Render preview based of entity domain #21926
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce a new private method, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EntityPreviewRow
participant RenderLogic
User->>EntityPreviewRow: Request to render entity
EntityPreviewRow->>RenderLogic: Call renderEntityState(stateObj)
RenderLogic-->>EntityPreviewRow: Return rendered component
EntityPreviewRow-->>User: Display rendered entity
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 (
|
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: 0
Outside diff range, codebase verification and nitpick comments (1)
src/dialogs/config-flow/previews/entity-preview-row.ts (1)
102-310
: Excellent work on centralizing the rendering logic for various entity states!The introduction of the
renderEntityState
method significantly improves code organization and readability by encapsulating the rendering logic for each entity type. The modular approach allows for easier future modifications and enhancements.The method supports a wide range of entity types, providing a comprehensive solution for rendering entity states. It utilizes appropriate UI components for each entity type, ensuring a consistent and user-friendly experience. The new CSS styles enhance the layout and appearance of the rendered components, and the importation of new components expands the functionality to support more diverse entity interactions.
To further improve the architecture and maintainability of the component, consider the following suggestions:
Extract the rendering logic for each entity type into separate methods or components. This will make the
renderEntityState
method more concise and easier to understand, and it will allow for better reusability and testability of the individual rendering logic.Create a mapping object that maps each domain to its corresponding rendering method or component. This will eliminate the need for the long chain of if statements and make it easier to add or modify the rendering logic for each domain.
Consider creating a base component for rendering entity states that encapsulates common functionality and properties. This will reduce duplication and make it easier to maintain consistency across different entity types.
Use a type system, such as TypeScript, to define the expected properties and types for each entity type. This will provide better type safety and make it easier to catch errors at compile time.
Add unit tests for each rendering method or component to ensure that they are functioning as expected and to catch any regressions.
By implementing these suggestions, you can further improve the architecture and maintainability of the component, making it easier to add new features and fix bugs in the future.
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.
No need to use html whenever you are just returning a string.
b092b7e
to
946dfdc
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.
Actionable comments posted: 4
Outside diff range and nitpick comments (1)
src/dialogs/config-flow/previews/entity-preview-row.ts (1)
68-106
: Clean Up Unused CSS StylesReview the CSS styles added between lines 68-106. If any styles are not utilized in the current implementation, consider removing them to maintain a clean and efficient stylesheet.
Not sure if you have already done this at this time, I've went through the code again and tested. |
I got stuck on trying to implement |
946dfdc
to
9e7bc57
Compare
@silamon I think this should be ready to go now 👍 |
Proposed change
Render preview based off the domain of the entity (currently all looks like a sensor).
I think probably needs to walk this though again to make sure all relevant ones are added (and possibly maybe remove something I could guess). Not all can be tested as there is limited preview entities currently.
Examples:
Type of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed:
Summary by CodeRabbit