-
Notifications
You must be signed in to change notification settings - Fork 209
Alignment to original HA popup. #1098
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
base: dev
Are you sure you want to change the base?
Conversation
dobby5
commented
Nov 9, 2025
- Fixed the bug that left a margin on the left and right sides. (Mobile view)
- Fixed the responsive behavior.
- Now the distance between the popup and the browser window is calculated and transferred to the mobile view.
- Fixed scrolling
- The background no longer scrolls when you continue scrolling at the end or beginning of the popup.
- Minor design adjustments
… view) Fixed the responsive behavior. Now the distance between the popup and the browser window is calculated and transferred to the mobile view. Fixed scrolling The background no longer scrolls when you continue scrolling at the end or beginning of the popup. Minor design adjustments
|
Hey @thomasloven, would you give me some feedback? I've implemented some of your requests. |
|
Hi @dobby5, you responded to my requests. Busy this week and I should be able to look at later in the week. |
dcapslock
left a comment
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.
Thank you again for refining the PR to this stage. However there is still is so much irrelevant changes in this PR that it is impossible to review. Comments left so far are to do with all the irrelevant changes and in some cases where a bug is introduced. I am fearful there are more bugs introduced that I cannot see.
Also, I see that you are abstracting the concept of dialog width from styles. This is not the approach needed. Diaolog width is part of styles. If the PR was focussed just on this part I might be able to give relevant feedback. However as to all the irrelevant changes, it is hard to pick out what is specifically regarding the dialog sizing.
I have stopped reviewing at this stage. Please confine this PR to the exact elements required to address the dialog alignment. Thank you.
js/plugin/popup-card.ts
Outdated
| ev.preventDefault(); | ||
| let properties = { ...cardConfig } | ||
| delete properties.card; | ||
| window.browser_mod?.service("popup", { |
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.
This is reverting a recent fix. Please update your branch.
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.
done
| import { BrowserModTileCardEditor } from "./tile-card-editor"; | ||
| import structuredClone from "@ungap/structured-clone"; | ||
| import { getLovelaceRoot } from "../../helpers"; | ||
| import { LovelaceGridOptions, LovelaceCard } from "../../types/types"; |
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 move types.
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.
Not necessary, but it would be nice, because it would mean that the project would at least follow the structural standards to some respects.
js/plugin/popups.ts
Outdated
|
|
||
| export function setCustomElementClass(dialogTag: string): void { | ||
| if (customElementClassCache[dialogTag]) { | ||
| // Check if already registered in customElements registry (not just cache) |
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.
All these changes for custom class are unneccessary. If they are to fix any issue (unlikely) then please submit as separate PR.
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.
Okay
tsconfig.json
Outdated
| "experimentalDecorators": true | ||
| } | ||
| } | ||
| }, |
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.
Changes only needed as you moved types. Leave types as is, or if critical to move, subit a separate PR moving types and updating build CI files.
|
|
||
| export interface HaButtonElement extends HTMLElement, HaButtonProps { } | ||
|
|
||
| export type DialogWidth = "small" | "medium" | "large" | "full"; |
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 am yet to look at this in detail, but a DialogWidth goes against the concept that the width of the dialog is part of the style. Please make sure you understand the relationship of dialog width being part of style and not an isolated feature.
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 think we are talking at cross-purposes, or perhaps expectations have changed over time. If that is the case, please let me know.
You originally wanted to adopt ha-wa-dialoge. If you take a look at the code, you will see that those types are integrated.
I have no idea what you will do with the types in the future, and I don't care.
Currently, it is used to calculate the dialog size. Until ha-wa-dialoge is used for the dashboard, this will remain a workaround.
js/plugin/popup-dialog.ts
Outdated
| flex: 1; | ||
| } | ||
| :host([card]) .content .container { |
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.
As you have changed card from a refectled property to _card, this syle will never be applied. So a bug introduced here due to the irrelevant changes you are applying without full understanding of their consequences.
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.
Paste mistake. Thank you.
| @state() _popupStyles: PopupStyle[] = []; | ||
| @state() _formdata: any; | ||
| @state() _formDataValid: boolean = true; | ||
| @state() _card: boolean = false; |
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.
Change from card to _card has styling consequences. Another irrelevant changes, and in this case I can immediately see it will introduced a bug.
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.
This is not irrelevant when working with standardization. Often, the difference between state, property, private, and public was not recognized or understood, but this is being fixed here.
A little tip in this regard: The Home Assistant frontend code is very nicely structured and invites you to learn from it.
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.
It would be helpful if you could tell me which bug you are seeing so that it can be fixed.
| @property({ type: String }) width: DialogWidth = "medium"; | ||
| @property({ type: Boolean }) preventScrimClose: boolean = false; | ||
| @property({ type: String }) headerTitle: string = ""; | ||
| @property({ type: String }) headerSubtitle: string = ""; |
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.
Where is headerSubtitle and headerSubtitlePosition used?
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.
See above: Preparation for adoption ha-wa-dialog
js/plugin/popup-dialog.ts
Outdated
| card.addEventListener("ll-rebuild", () => { | ||
| this._build_card(config); | ||
| }); | ||
| } catch (error) { |
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. This is not required. The error is caught by Frontend which will display error title and details in preview mode, and 'Configuration error' otherwise.
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.
Changed
js/plugin/popup-dialog.ts
Outdated
| } | ||
| }); | ||
| } | ||
| if (changedProperties.has("_narrow")) { |
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.
Just make 'narrow` refelected attribute.
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 Idea what you mean exactly but maybe this...
|
@dobby5 will review later this week or over the weekend when time permits. |
Do you have any Feedback for me? |