Skip to content
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 storage variables for application credentials config table #21215

Merged
merged 1 commit into from
Jun 28, 2024

Conversation

silamon
Copy link
Contributor

@silamon silamon commented Jun 28, 2024

Proposed change

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Summary by CodeRabbit

  • New Features
    • Added event handling for sorting and column changes in the data table component.
    • Implemented state persistence for sorting, column order, and hidden columns to enhance user experience.

This comment was marked as spam.

@silamon

This comment was marked as off-topic.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Outside diff range comments (1)
src/panels/config/application_credentials/ha-config-application-credentials.ts (1)

Line range hint 245-245: Specify a more precise type instead of any.

Using any can lead to potential bugs due to lack of type safety. Specify a more explicit type for the error in the catch block.

- } catch (err: any) {
+ } catch (err: Error) {

@@ -27,6 +28,7 @@ import type { HaTabsSubpageDataTable } from "../../../layouts/hass-tabs-subpage-
import { HomeAssistant, Route } from "../../../types";
import { configSections } from "../ha-panel-config";
import { showAddApplicationCredentialDialog } from "./show-dialog-add-application-credential";
import { storage } from "../../../common/decorators/storage";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure consistent use of relative imports.

The import for storage uses a relative path that is quite deep. Consider using a more consistent or simplified path if possible, especially if there's a base path configuration that can be leveraged.

Comment on lines +283 to +290
private _handleSortingChanged(ev: CustomEvent) {
this._activeSorting = ev.detail;
}

private _handleColumnsChanged(ev: CustomEvent) {
this._activeColumnOrder = ev.detail.columnOrder;
this._activeHiddenColumns = ev.detail.hiddenColumns;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor event handler methods for clarity and efficiency.

The methods _handleSortingChanged and _handleColumnsChanged directly mutate the state properties. Consider using a more declarative approach or ensuring that these mutations trigger the necessary updates in the UI.

-  this._activeSorting = ev.detail;
+  this._setSorting(ev.detail);

-  this._activeColumnOrder = ev.detail.columnOrder;
-  this._activeHiddenColumns = ev.detail.hiddenColumns;
+  this._setColumnConfig(ev.detail.columnOrder, ev.detail.hiddenColumns);

And implement the methods _setSorting and _setColumnConfig to handle the state updates more cleanly.

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.

Suggested change
private _handleSortingChanged(ev: CustomEvent) {
this._activeSorting = ev.detail;
}
private _handleColumnsChanged(ev: CustomEvent) {
this._activeColumnOrder = ev.detail.columnOrder;
this._activeHiddenColumns = ev.detail.hiddenColumns;
}
private _handleSortingChanged(ev: CustomEvent) {
this._setSorting(ev.detail);
}
private _handleColumnsChanged(ev: CustomEvent) {
this._setColumnConfig(ev.detail.columnOrder, ev.detail.hiddenColumns);
}

@silamon silamon added this to the 2024.7 milestone Jun 28, 2024
Copy link
Member

@piitaya piitaya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @silamon

@piitaya piitaya merged commit 4a1087c into home-assistant:dev Jun 28, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Customizing columns: Application Credentials customizations not saved
2 participants