Add a picture uploader to picture-card-editor #18695
Conversation
|
Oh I do like this!
Yes, that would be a good idea, similar to the
That also sounds good, but should be options on the selector then probably? The last option would need backend support first. (We do need something similar for uploaded pictures though probably, as there is no way to manage or reuse already uploaded images) |
|
If we had image-selector, what do you think of how the current UI looks? OK to just have a Image browser/picker could come later if/when that ever gets implemented. |
|
I like this one too! I'm seeing 3 options to select an image, correct me if I'm wrong:
In my opinion, the input-select with image URL isn't user friendly and pretty advanced. You have to know the path to a file, that requires access to your system. Can we drop this one? That means there should be an option to select an image from the system, like we're doing with the Media panel. |
The URL input can also be remote URL on the web, it doesn't have to be local. |
|
I think I would hide the input by default, but allow the user to switch to it with a button |
That is the third option in my mind. So it would be:
|
I agree. There should be a way to select between selecting a local (already uploaded) file, upload new image or upload image from the web. This last option only have an input. The other doesn't need one in my mind. |
Can you clarify, what is "the input" you're referring to hide, the text selector, or the picture-upload? I was kind of thinking to have the button toggle the larger upload element, though I'm wondering if Matthias is suggesting the opposite; to hide the URL. I'm not sure if that makes sense to me yet. This is kind of what I was going to go for:
Matthias, you seem to also be suggesting that remote URLs should be replaced with "upload to a local image from web", but I think people might not necessarily want to upload remote URLs. The behavior now is just to show the live image from the remote domain. Maybe that is desired, in case the remote image is dynamic, or user just don't want to store it on his server? |
|
@matthiasdebaat can you check above questions? |
|
Thanks for your feedback. Based on this I've got something like this in my mind. More details can be found in the Figma design.
My suggestions are:
|
|
Updated from feedback. I've for now dropped the A better flow might be to have first the entire picture uploaded without crop, and then have a
|
|
We should add the camera entity and image entity to that radio list, as that will override the image (the entity will too btw on a |
src/components/ha-picture-upload.ts
Outdated
| )} | ||
| > | ||
| </ha-button> | ||
| ${this.canDelete |
There was a problem hiding this comment.
I don't think it is clear to a user what the delete button does, it looks like it does the same as the change picture button, but it deletes it from the server...
We should have a picker of current uplloaded files sometime 😓
There was a problem hiding this comment.
I would offer to remove the delete button, but it was just added on Matthias's request 😕
The other weird thing about delete is that it's in a cancellable dialog. If you delete the picture, but then cancel the change, the picture becomes a broken link to a no longer existing image.
I assumed that more picture management functionality would maybe come someday in the future, but I felt maybe this was ok to go ahead without that, as that felt like might be a pretty big project with new APIs.
There was a problem hiding this comment.
For the sake of not blocking this PR, I'm okay if the delete button is removed.
We should have a picker of current uplloaded files sometime 😓
I agree
There was a problem hiding this comment.
For the sake of not blocking this PR, I'm okay if the delete button is removed.
It's gone.
bramkragten
force-pushed
the
picture-upload
branch
from
94aa3b8 to
aaaf3af
Compare
WalkthroughWalkthroughThe updates introduce a new image selector component ( Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant HaImageSelector
participant HaPictureUpload
participant ImageUploadAPI
User->>HaImageSelector: Select image option
HaImageSelector->>User: Show upload/URL input
User->>HaImageSelector: Upload image / Enter URL
HaImageSelector->>HaPictureUpload: Upload image
HaPictureUpload->>ImageUploadAPI: Send image data
ImageUploadAPI->>HaPictureUpload: Confirm upload
HaPictureUpload->>HaImageSelector: Return image URL
HaImageSelector->>User: Display selected image
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (10)
Additional Context UsedBiome (58)
Additional comments not posted (10)
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
I would still really like to combine the image entity/camera entity/upload image/image url options for improved clarity that they can not be used together. But this is already a nice addition that we should merge. Thanks @karwosts! |
![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 comments (1)
src/components/ha-selector/ha-selector.ts (1)
Line range hint
72-72: Specify explicit types instead of usingany.- @property() public value?: any; + @property() public value?: string | number | YourCustomType; // Adjust the type according to expected values - @property() public placeholder?: any; + @property() public placeholder?: string; // Adjust the type if necessary - @property({ attribute: false }) public context?: Record<string, any>; + @property({ attribute: false }) public context?: Record<string, YourSpecificType>; // Define 'YourSpecificType' as neededUsing
anycan lead to less predictable code and more difficult debugging. Specifying explicit types improves type safety and code clarity.Also applies to: 81-81, 87-87
| "image": { | ||
| "select_image": "Select image", | ||
| "upload": "Upload picture", | ||
| "url": "Local path or web URL" | ||
| }, |
There was a problem hiding this comment.
This section appears to be a duplicate of the earlier translation keys for image handling. Please verify and consider removing if it's unintentional.
| export interface ImageSelector { | ||
| // eslint-disable-next-line @typescript-eslint/ban-types | ||
| image: {} | null; | ||
| } |
There was a problem hiding this comment.
Define a more specific type for ImageSelector.
- export interface ImageSelector {
- image: {} | null;
- }
+ export interface ImageSelector {
+ image: {
+ url?: string;
+ altText?: string;
+ } | null;
+ }Using {} as a type is generally discouraged as it is too broad and does not provide useful type checking. Defining a more specific type will enhance maintainability and clarity of the code.
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.
| export interface ImageSelector { | |
| // eslint-disable-next-line @typescript-eslint/ban-types | |
| image: {} | null; | |
| } | |
| export interface ImageSelector { | |
| image: { | |
| url?: string; | |
| altText?: string; | |
| } | null; | |
| } |
| export const getIdFromUrl = (url: string): string | undefined => { | ||
| let id; | ||
| if (url.startsWith(URL_PREFIX)) { | ||
| id = url.substring(URL_PREFIX.length); | ||
| const idx = id.indexOf("/"); | ||
| if (idx >= 0) { | ||
| id = id.substring(0, idx); | ||
| } | ||
| } | ||
| return id; |
There was a problem hiding this comment.
Consider adding explicit type annotations to enhance code clarity and maintainability. For instance, id should be explicitly typed as string | undefined.
- let id;
+ let id: string | undefined;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.
| export const getIdFromUrl = (url: string): string | undefined => { | |
| let id; | |
| if (url.startsWith(URL_PREFIX)) { | |
| id = url.substring(URL_PREFIX.length); | |
| const idx = id.indexOf("/"); | |
| if (idx >= 0) { | |
| id = id.substring(0, idx); | |
| } | |
| } | |
| return id; | |
| export const getIdFromUrl = (url: string): string | undefined => { | |
| let id: string | undefined; | |
| if (url.startsWith(URL_PREFIX)) { | |
| id = url.substring(URL_PREFIX.length); | |
| const idx = id.indexOf("/"); | |
| if (idx >= 0) { | |
| id = id.substring(0, idx); | |
| } | |
| } | |
| return id; |
| protected render() { | ||
| return html` | ||
| <div> | ||
| <label> | ||
| ${this.hass.localize("ui.components.selectors.image.select_image")} | ||
| <ha-formfield | ||
| .label=${this.hass.localize("ui.components.selectors.image.upload")} | ||
| > | ||
| <ha-radio | ||
| name="mode" | ||
| value="upload" | ||
| .checked=${this.showUpload} | ||
| @change=${this._radioGroupPicked} | ||
| ></ha-radio> | ||
| </ha-formfield> | ||
| <ha-formfield | ||
| .label=${this.hass.localize("ui.components.selectors.image.url")} | ||
| > | ||
| <ha-radio | ||
| name="mode" | ||
| value="url" | ||
| .checked=${!this.showUpload} | ||
| @change=${this._radioGroupPicked} | ||
| ></ha-radio> | ||
| </ha-formfield> | ||
| </label> | ||
| ${!this.showUpload | ||
| ? html` | ||
| <ha-textfield | ||
| .name=${this.name} | ||
| .value=${this.value || ""} | ||
| .placeholder=${this.placeholder || ""} | ||
| .helper=${this.helper} | ||
| helperPersistent | ||
| .disabled=${this.disabled} | ||
| @input=${this._handleChange} | ||
| .label=${this.label || ""} | ||
| .required=${this.required} | ||
| ></ha-textfield> | ||
| ` | ||
| : html` | ||
| <ha-picture-upload | ||
| .hass=${this.hass} | ||
| .value=${this.value?.startsWith(URL_PREFIX) ? this.value : null} | ||
| @change=${this._pictureChanged} | ||
| ></ha-picture-upload> | ||
| `} | ||
| </div> | ||
| `; | ||
| } | ||
|
|
||
| private _radioGroupPicked(ev): void { | ||
| this.showUpload = ev.target.value === "upload"; | ||
| } | ||
|
|
||
| private _pictureChanged(ev) { | ||
| const value = (ev.target as HaPictureUpload).value; | ||
|
|
||
| fireEvent(this, "value-changed", { value: value ?? undefined }); | ||
| } | ||
|
|
||
| private _handleChange(ev) { | ||
| let value = ev.target.value; | ||
| if (this.value === value) { | ||
| return; | ||
| } | ||
| if (value === "" && !this.required) { | ||
| value = undefined; | ||
| } | ||
|
|
||
| fireEvent(this, "value-changed", { value }); | ||
| } | ||
|
|
||
| static get styles(): CSSResultGroup { | ||
| return css` | ||
| :host { | ||
| display: block; | ||
| position: relative; | ||
| } | ||
| div { | ||
| display: flex; | ||
| flex-direction: column; | ||
| } | ||
| label { | ||
| display: flex; | ||
| flex-direction: column; | ||
| } | ||
| ha-textarea, | ||
| ha-textfield { | ||
| width: 100%; | ||
| } | ||
| `; | ||
| } | ||
| } | ||
|
|
||
| declare global { | ||
| interface HTMLElementTagNameMap { | ||
| "ha-selector-image": HaImageSelector; | ||
| } | ||
| } |
There was a problem hiding this comment.
The new HaImageSelector component is well-implemented, providing a flexible UI component for image selection. It supports both upload and URL modes, which aligns with the PR's objectives. Consider specifying a more precise type than any for the value property to enhance type safety.
- @property() public value?: any;
+ @property() public value?: string | null; // Adjust as appropriate based on expected typesCommittable 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.
| @customElement("ha-selector-image") | |
| export class HaImageSelector extends LitElement { | |
| @property({ attribute: false }) public hass!: HomeAssistant; | |
| @property() public value?: any; | |
| @property() public name?: string; | |
| @property() public label?: string; | |
| @property() public placeholder?: string; | |
| @property() public helper?: string; | |
| @property({ attribute: false }) public selector!: ImageSelector; | |
| @property({ type: Boolean }) public disabled = false; | |
| @property({ type: Boolean }) public required = true; | |
| @state() private showUpload = false; | |
| protected firstUpdated(changedProps): void { | |
| super.firstUpdated(changedProps); | |
| if (!this.value || this.value.startsWith(URL_PREFIX)) { | |
| this.showUpload = true; | |
| } | |
| } | |
| protected render() { | |
| return html` | |
| <div> | |
| <label> | |
| ${this.hass.localize("ui.components.selectors.image.select_image")} | |
| <ha-formfield | |
| .label=${this.hass.localize("ui.components.selectors.image.upload")} | |
| > | |
| <ha-radio | |
| name="mode" | |
| value="upload" | |
| .checked=${this.showUpload} | |
| @change=${this._radioGroupPicked} | |
| ></ha-radio> | |
| </ha-formfield> | |
| <ha-formfield | |
| .label=${this.hass.localize("ui.components.selectors.image.url")} | |
| > | |
| <ha-radio | |
| name="mode" | |
| value="url" | |
| .checked=${!this.showUpload} | |
| @change=${this._radioGroupPicked} | |
| ></ha-radio> | |
| </ha-formfield> | |
| </label> | |
| ${!this.showUpload | |
| ? html` | |
| <ha-textfield | |
| .name=${this.name} | |
| .value=${this.value || ""} | |
| .placeholder=${this.placeholder || ""} | |
| .helper=${this.helper} | |
| helperPersistent | |
| .disabled=${this.disabled} | |
| @input=${this._handleChange} | |
| .label=${this.label || ""} | |
| .required=${this.required} | |
| ></ha-textfield> | |
| ` | |
| : html` | |
| <ha-picture-upload | |
| .hass=${this.hass} | |
| .value=${this.value?.startsWith(URL_PREFIX) ? this.value : null} | |
| @change=${this._pictureChanged} | |
| ></ha-picture-upload> | |
| `} | |
| </div> | |
| `; | |
| } | |
| private _radioGroupPicked(ev): void { | |
| this.showUpload = ev.target.value === "upload"; | |
| } | |
| private _pictureChanged(ev) { | |
| const value = (ev.target as HaPictureUpload).value; | |
| fireEvent(this, "value-changed", { value: value ?? undefined }); | |
| } | |
| private _handleChange(ev) { | |
| let value = ev.target.value; | |
| if (this.value === value) { | |
| return; | |
| } | |
| if (value === "" && !this.required) { | |
| value = undefined; | |
| } | |
| fireEvent(this, "value-changed", { value }); | |
| } | |
| static get styles(): CSSResultGroup { | |
| return css` | |
| :host { | |
| display: block; | |
| position: relative; | |
| } | |
| div { | |
| display: flex; | |
| flex-direction: column; | |
| } | |
| label { | |
| display: flex; | |
| flex-direction: column; | |
| } | |
| ha-textarea, | |
| ha-textfield { | |
| width: 100%; | |
| } | |
| `; | |
| } | |
| } | |
| declare global { | |
| interface HTMLElementTagNameMap { | |
| "ha-selector-image": HaImageSelector; | |
| } | |
| } | |
| @property() public value?: string | null; |
Proposed change
Add a picture-upload form to the picture-card editor. I think this makes working with images much more accessible, otherwise it can be a source of a lot of confusion how to add pictures to the server and figure out the path.
Maybe this should be wrapped up in some kind of image-selector element that can be reused?
That would allow for either entering an external URL, uploading, or maybe opening a file picker and allowing to select from the list of current uploaded images either in /image/ or /www/?
Type of change
Example configuration
Additional information
Picture File Locations in Dashboard cards: Use the Settings: Areas & Zones drag/drop dialog. #18669
Checklist
If user exposed functionality or configuration variables are added/changed:
Summary by CodeRabbit
New Features
Style
Translations