-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Devtools statistics - new style, multi-select, & multi-delete #21813
Devtools statistics - new style, multi-select, & multi-delete #21813
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 involve substantial updates to the Changes
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: 1
@@ -54,10 +91,42 @@ class HaPanelDevStatistics extends SubscribeMixin(LitElement) { | |||
|
|||
@state() private _data: StatisticData[] = [] as StatisticsMetaData[]; | |||
|
|||
@state() private filter = ""; | |||
|
|||
@state() private _selected?; |
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.
Consider specifying a more specific type for _selected
.
Using any[]
bypasses type checking. If possible, specify a more precise type for better type safety.
- @state() private _selected?: any[];
+ @state() private _selected?: StatisticData[];
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.
@state() private _selected?; | |
@state() private _selected?: StatisticData[]; |
const localize = this.hass.localize; | ||
const columns = this._columns(this.hass.localize); | ||
|
||
const selectModeBtn = !this._selectMode |
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.
We should not duplicate all the logic from hass-tabs-subpage-data-table
we should either use that element if possible and other wise extract it to another element so it can be reused in multiple elements.
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.
Any thought which way I should try to go? I had the same thought but couldn't really figure out how to make it work.
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 would say a new element that can be used by both hass-tabs-subpage-data-table
and other places like here.
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.
That's probably going to be more difficult than I can tackle. Guess I'll drop this as I don't see a way forward.
974ea95
to
6b6482f
Compare
I re-opened and fixed this one (without the refactor for now) as it can be nice to have with the issues as repair issues now. I did change 1 thing, instead of multi fix, I made it a multi delete feature instead of a multi fix one. |
Proposed change
Following from the discussions of #20389 I tried to implement multi-select & bulk-issue fixing for developer tools/statistics.
The existing table was getting a bit rough and buggy, not working gracefully in narrow mode, so also decided to try to pickup some of the new data tables enhancements. It seems to work well, though I had to do an ugly amount of copy-pasting, given that there was no way I could see to leverage the existing code in hass-tabs-subpage-data-table. Tried looking to see if developer tools could use a hass-tabs style setup instead of paper tabs, but hass-tabs-subpage can't cleanly fit all of our devtools tabs, so just kept with the existing paper tabs and tried to hack in the table features.
Now table supports sorting, grouping, customizing columns, multi-select, and bulk issue fixing (for all types except units_changed which I haven't decided what to do with yet). But those are probably less common type of issues so I don't think it's a big loss.
Looked at getting filters as well, but the existing multi-pane structure was too complex to replicate and not worth the effort so I punted on that. I don't think it's needed yet.
Type of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation