Skip to content

added title to view configuation#4037

Merged
bramkragten merged 6 commits into
home-assistant:devfrom
drakeloud:feature/fixing-view-config
Oct 22, 2019
Merged

added title to view configuation#4037
bramkragten merged 6 commits into
home-assistant:devfrom
drakeloud:feature/fixing-view-config

Conversation

@drakeloud
Copy link
Copy Markdown
Contributor

Screen Shot 2019-10-17 at 9 51 16 AM

Fixed issue #4025

Added the config title to the card!

@homeassistant
Copy link
Copy Markdown
Contributor

Hi @drakeloud,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

return title;
}

return `${this._config.title} ${title}`;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should provide the title as a variable to the translation. So the translator can move the order of the words when needed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@bramkragten If we make the title as a translation variable though, then we wouldn't be able to data bind to the title field, and include the 'view configuration'

Copy link
Copy Markdown
Member

@bramkragten bramkragten Oct 20, 2019

Choose a reason for hiding this comment

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

Not sure I understand what you are saying, I suggest:

this._config.title ? 
    this.hass!.localize("ui.panel.lovelace.editor.edit_view.header_name", name, ${this._config.title}) : 
    this.hass!.localize("ui.panel.lovelace.editor.edit_view.header")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I am following either. Because config.title is a dynamic value, it doesn't seem like we can move that to the translation variables like you mentioned, so how do you suggest that we allow the translator to change the order of the translation for that value?

Oh wait a second. So you are suggesting passing all of that in to the localize function, and having the translator take care of it?

Where is the code for the localize function to change the parameters for the function? Perhaps I am confused about how the hass.localize function works.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can pass arguments to the localize function, and add placeholders ({placeholder}) to the translations text. These placeholders are replaced by the values we pass in as arguments.

@drakeloud
Copy link
Copy Markdown
Contributor Author

drakeloud commented Oct 21, 2019

@bramkragten I updated my PR. Is this what you were meaning?

);;
}

return this.hass!.localize("ui.panel.lovelace.editor.edit_view.header_name", this._config.title)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does the key header_name exist? You have to add it to src/translations/en.json: Edit view {name}
And you have to pass the name of the placeholder you want to replace:

Suggested change
return this.hass!.localize("ui.panel.lovelace.editor.edit_view.header_name", this._config.title)
return this.hass!.localize("ui.panel.lovelace.editor.edit_view.header_name", name, this._config.title)

}

private get _viewConfigTitle(): string {
if (!this._config || !this._config.title || this._config.title === "") {
Copy link
Copy Markdown
Member

@bramkragten bramkragten Oct 21, 2019

Choose a reason for hiding this comment

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

If there is no config we might want to change the header to Add new view?

}

private get _viewConfigTitle(): string {
if (!this._config || !this._config.title || this._config.title === "") {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

An empty string is falsy in javascript so this._config.title === "" is not needed.

Suggested change
if (!this._config || !this._config.title || this._config.title === "") {
if (!this._config || !this._config.title) {

@drakeloud
Copy link
Copy Markdown
Contributor Author

@bramkragten Ah I see. I admittedly have never done any work before around localization, so this has been all new for me. Thanks for the tips along the way.

I have updated the PR with your suggestions.

Copy link
Copy Markdown
Member

@bramkragten bramkragten left a comment

Choose a reason for hiding this comment

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

Thanks!

@bramkragten bramkragten merged commit 7b52015 into home-assistant:dev Oct 22, 2019
@bramkragten bramkragten mentioned this pull request Oct 23, 2019
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants