Replace polymer paper-tabs #22018
Replace polymer paper-tabs #22018
Conversation
github-actions
bot
added
Demo
|
I like it ! Good job ! I noticed 2 small thing on the dashboard tabs :
|
|
There's a lot of polish work to be done, I'm currently looking if the scrolling and functional behavior is close to be OK. |
|
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 significant modifications to the theme configuration and the introduction of new custom components in the project. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PanelDeveloperTools
participant HaMdTabs
participant HaMdSecondaryTab
User->>PanelDeveloperTools: Selects tab
PanelDeveloperTools->>HaMdTabs: Update active tab index
HaMdTabs->>HaMdSecondaryTab: Render selected tab
HaMdSecondaryTab->>User: Display tab content
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: 4
Outside diff range and nitpick comments (5)
src/components/ha-md-tabs.ts (4)
4-5: Remove unused imports to clean up the codebaseThe components
ha-icon-button-prevandha-icon-button-nextare imported but not used in this file. Since the left and right arrows have been removed as per the PR objectives, these imports are no longer necessary and can be safely deleted.Apply the following diff to remove the unused imports:
-import "./ha-icon-button-prev"; -import "./ha-icon-button-next";
10-11: Remove unused property 'hass' if it's not neededThe property
hassis declared but not used within this component. If it's not required, consider removing it to simplify the code.Apply the following diff to remove the unused property:
export class HaMdTabs extends MdTabs { - @property({ attribute: false }) public hass!: HomeAssistant;
27-27: Refine type assertions to enhance type safetyIn the expressions
(slider! as any).offsetLeft, the use ofas anyand non-null assertions bypasses TypeScript's type checking, potentially hiding issues. Consider refining the type ofsliderto avoid casting toanyand using non-null assertions.Specify the type of
sliderasHTMLElement:- const slider = this.shadowRoot?.querySelector(".tabs"); + const slider = this.shadowRoot?.querySelector(".tabs") as HTMLElement | null;Then, you can remove
as anyand the non-null assertion:- startX = e.pageX - (slider! as any).offsetLeft; + startX = e.pageX - slider.offsetLeft; - const x = e.pageX - (slider! as any).offsetLeft; + const x = e.pageX - slider.offsetLeft;Also applies to: 49-49
61-74: Combine duplicate ':host' selectors in CSS for cleaner codeThere are two
:hostselectors in the styles. Consider combining them into a single:hostblock to improve readability and maintainability.Apply the following diff:
:host { --md-sys-color-primary: var(--primary-color); --md-sys-color-secondary: var(--secondary-color); --md-sys-color-surface: var(--card-background-color); --md-sys-color-on-surface: var(--primary-color); --md-sys-color-on-surface-variant: var(--secondary-color); --md-divider-thickness: 0px; --md-primary-tab-container-height: 56px; --md-secondary-tab-container-height: 56px; + scroll-behavior: unset; } - :host { - scroll-behavior: unset; - }src/panels/lovelace/hui-root.ts (1)
466-466: Review the use of 'slot="icon"' on 'ha-icon-button'At line 466, the
ha-icon-buttonincludes aslot="icon"attribute. Unless this button is intended to be placed into aniconslot of a parent component, theslotattribute may be unnecessary.Apply this diff if the
slot="icon"is not needed:-<ha-icon-button - id="add-view" - @click=${this._addView} - .label=${this.hass!.localize( - "ui.panel.lovelace.editor.edit_view.add" - )} - .path=${mdiPlus} - slot="icon" -></ha-icon-button> +<ha-icon-button + id="add-view" + @click=${this._addView} + .label=${this.hass!.localize( + "ui.panel.lovelace.editor.edit_view.add" + )} + .path=${mdiPlus} +></ha-icon-button>
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 (11)
- demo/src/configs/jimpower/theme.ts (0 hunks)
- demo/src/configs/kernehed/theme.ts (0 hunks)
- package.json (0 hunks)
- src/components/ha-md-primary-tab.ts (1 hunks)
- src/components/ha-md-secondary-tab.ts (1 hunks)
- src/components/ha-md-tabs.ts (1 hunks)
- src/components/ha-tabs.ts (0 hunks)
- src/panels/developer-tools/ha-panel-developer-tools.ts (7 hunks)
- src/panels/lovelace/editor/config-elements/hui-conditional-card-editor.ts (4 hunks)
- src/panels/lovelace/editor/config-elements/hui-stack-card-editor.ts (4 hunks)
- src/panels/lovelace/hui-root.ts (10 hunks)
Files not reviewed due to no reviewable changes (4)
- demo/src/configs/jimpower/theme.ts
- demo/src/configs/kernehed/theme.ts
- package.json
- src/components/ha-tabs.ts
Additional comments not posted (21)
src/components/ha-md-primary-tab.ts (1)
1-11: LGTM!The code correctly defines a new custom element
ha-md-primary-tabthat extends theMdPrimaryTabclass from the@material/weblibrary. The custom element is properly defined using the@customElementdecorator and declared in the globalHTMLElementTagNameMapinterface.The custom element does not add any new functionality or modify the behavior of the base class, making it a simple wrapper around the
MdPrimaryTabclass.src/components/ha-md-secondary-tab.ts (1)
1-11: LGTM!The code segment correctly defines a new custom element
ha-md-secondary-tabby extending theMdSecondaryTabclass from the@material/weblibrary. It follows the proper syntax for defining custom elements using thelitlibrary and declares the custom element in the globalHTMLElementTagNameMapinterface for type checking and autocompletion.The use of the
@material/weblibrary ensures consistency with the Material Design guidelines and provides a reliable implementation of the secondary tab component.src/panels/lovelace/editor/config-elements/hui-conditional-card-editor.ts (4)
26-26: LGTM!The import statement for the
ha-md-secondary-tabcomponent is correctly added.
81-95: LGTM!The tab components have been correctly replaced with the new
ha-md-tabsandha-md-secondary-tabcomponents. The event handling and label localization are also updated accordingly.
159-160: LGTM!The
_selectTabmethod signature and event handling logic are correctly updated to work with the new tab component.
260-262: LGTM!The CSS styles for the
ha-md-tabscomponent are correctly added, including the text transformation to uppercase.src/panels/lovelace/editor/config-elements/hui-stack-card-editor.ts (4)
42-43: LGTM!The imports for the new custom tab components look good.
122-134: Looks good!The transition from Polymer tab components to the new custom tab components has been implemented correctly. The usage of
activeTabIndex,@changeevent, andha-md-secondary-tabcomponent is appropriate.
223-223: LGTM!The selected card index is correctly updated based on the
activeTabIndexproperty of the new custom tab component.
322-322: Looks good!The toolbar width has been appropriately set to 100%.
src/panels/developer-tools/ha-panel-developer-tools.ts (4)
67-81: Implementation ofha-md-tabsenhances tab navigationThe use of the
ha-md-tabscomponent with proper binding ofactiveTabIndexand event handling improves the tab navigation experience and aligns with Material Design guidelines.
91-97: Correct handling of tab change events in_handleTabChangedThe event handler appropriately updates
_activeTabIndexand navigates to the new route when the selected tab changes, ensuring the UI stays in sync with the application state.
158-167: Styles forha-md-tabsare correctly appliedThe additional styles ensure that the
ha-md-tabscomponent integrates seamlessly with the application's theme, maintaining visual consistency across different components.
16-24: Verify thattab.labelmatches existing localization keysThe
labelproperty in thetabsarray is used to construct localization keys likeui.panel.developer-tools.tabs.${tab.label}.title. Please ensure that these keys exist for all tabs, especially for 'states' and 'actions', to guarantee that tab labels are correctly localized.Run the following script to check for the presence of the localization keys:
Verification successful
Localization keys for developer tools tabs are present and consistent
After thorough investigation, I can confirm that the localization keys for the developer tools tabs exist in the main translation file (src/translations/en.json). The keys follow the pattern "ui.panel.developer-tools.tabs.${tab.label}.title" as expected. Specifically:
- The 'yaml', 'states', 'actions', 'templates', 'events', 'statistics', and 'assist' labels used in the tabs array have corresponding entries in the translation file.
- The 'states' and 'actions' labels, which were of particular concern in the original comment, are present and correctly formatted.
There's no need for manual verification as the localization structure is consistent with the implementation. The review comment's concern about ensuring the existence of these keys has been addressed.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that localization keys for developer tools tabs exist. # Test: Search for localization keys. Expect: Keys to be defined in localization files. rg '("|\')ui\.panel\.developer-tools\.tabs\.(yaml|states|actions|templates|events|statistics|assist)\.title("|\')' -g '*.json'Length of output: 263
Script:
#!/bin/bash # Description: Verify that localization keys for developer tools tabs exist. # Test: Search for localization keys. Expect: Keys to be defined in localization files. rg "ui\.panel\.developer-tools\.tabs\.(yaml|states?|actions?|templates?|events?|statistics|assist)\.title" -g "*.json"Length of output: 120
Script:
#!/bin/bash # Description: Investigate localization file structure and content # Check for JSON files echo "Searching for JSON files:" fd -e json echo "\nSearching for other potential localization files:" fd -e yaml -e yml -e po echo "\nSearching for files with 'lang' or 'localization' in the name:" fd -g "*lang*" -g "*localization*" echo "\nSearching for 'developer-tools' string in all files:" rg "developer-tools" -g "!*.{lock,svg,png,jpg}"Length of output: 33318
src/panels/lovelace/hui-root.ts (7)
25-25: Importing 'nothing' from 'lit'The addition of the
nothingimport fromlitis appropriate for use in conditional rendering within templates.
51-53: Added imports for new Material Design tab componentsThe imports of
ha-md-tabs,ha-md-primary-tab, andha-md-secondary-tabcorrectly include the new Material Design tab components required for the replacement of Polymerpaper-tabs.
338-338: Verify correct usage of 'ha-md-tabs' properties and eventsThe
ha-md-tabscomponent is initialized with the.activeTabIndexproperty and the@changeevent handler. Ensure that:
- The
.activeTabIndexproperty correctly reflects the currently active tab index.- The
@changeevent is the appropriate event to listen for tab changes withha-md-tabs.- The
_handleViewSelectedmethod correctly handles the event.Also applies to: 340-342
1016-1017: Defined '.flexrow' CSS class for layoutThe addition of the
.flexrowclass withdisplay: flex;improves the layout styling for flex container elements.
1022-1024: Updated styles for 'ha-md-tabs' in edit modeThe CSS styles for
ha-md-tabsin edit mode are updated to match the editing theme, ensuring consistent appearance during editing.
1037-1039: Show edit icons only on active tabsThe CSS rule displays
.edit-iconelements only when their parentha-md-secondary-tabhas theactiveattribute. This helps maintain a clean interface by showing edit options only for the active tab.
1095-1113: Applied custom theming to 'ha-md-tabs'Custom CSS variables are set for
ha-md-tabsto align with the application's header text and background colors, ensuring cohesive theming across the interface.
|
This branch has been rebased, polish work done, but now awaiting review. |
There was a problem hiding this comment.
Thanks for this nice PR, looking forward when we have removed all of the polymer code.
Why do you use secondary tabs?
material says:
Primary tabs are placed at the top of the content pane under a top app bar. They display the main content destinations. material-web.dev/components/tabs
So I would say at least for hui-root and dev tools it should be primary instead of secondary. Unused imports of primary-tab or secondary-tab should be removed.
src/panels/lovelace/editor/config-elements/hui-conditional-card-editor.ts
Outdated
Show resolved
Hide resolved
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Co-authored-by: Wendelin <[email protected]>
Co-authored-by: Wendelin <[email protected]>
…d-editor.ts Co-authored-by: Wendelin <[email protected]>
Co-authored-by: Wendelin <[email protected]>
|
The secondary tabs matched the old polymer more in styling. I will change in the coming days. |
wendevlin
added
the
Needs UX
|
Hi @silamon, |
It can be added, but we will need to change the flex direction to make it work (more technically, override the html that draws the tabs) |
Ok, UX decided that we need the arrows, would you like to implement it as it was before? |
wendevlin
removed
the
Needs UX
|
Thanks @silamon. The buttons should only be visible when needed.
Same behaviour as the current tabs are using. |
We will need to replicate it as much as possible, but it's not going to be a copy paste since the md-tabs is using a different way of setup. I've added an attempt, but it's not yet the same feeling. |
Yeah it is a very difficult task and you have to take care of a lot of possibilities. Do you want to continue or should we close it and add it to our internal tasks? |
Proposed change
The left and right arrows are removed. Replaces the polymer paper-tabs with the new material3 md tabs.
Type of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed:
Summary by CodeRabbit
New Features
ha-md-primary-tab,ha-md-secondary-tab,ha-md-tabs) for improved navigation.HaMdTabscomponent.Bug Fixes
Refactor
paper-tabswith new Material Design components across various panels and editors.Chores